Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: expose file & fullName props on TestContext (for snapshots) #53179

Conversation

JakobJingleheimer
Copy link
Contributor

Per our discussion last month (in slack): These properties are needed to facilitate jest-like snapshots in node's test runner¹.

¹ I have a simple package for this that is nearly done (hoping to polish it off this week), which we intend to publish on npm under either the @pkgjs or @nodejs namespace.

@JakobJingleheimer JakobJingleheimer added the test_runner Issues and PRs related to the test runner subsystem. label May 27, 2024
@JakobJingleheimer JakobJingleheimer self-assigned this May 27, 2024
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/test_runner

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label May 27, 2024
@JakobJingleheimer JakobJingleheimer force-pushed the test_runner/expose-props-for-snapshots branch from 74992d7 to 9ae68a9 Compare May 27, 2024 21:09
@cjihrig
Copy link
Contributor

cjihrig commented May 27, 2024

Duplicate of #53169?

@JakobJingleheimer
Copy link
Contributor Author

JakobJingleheimer commented May 27, 2024

Dammit. I've been talking with the test runner team about this for like 2 months. Why did you go and implement this in parallel with no discussion or even a heads up to at least warn me you were about to waste all the time I spent on this and its related work.

@cjihrig
Copy link
Contributor

cjihrig commented May 27, 2024

I'm not really sure how to respond to that. I certainly didn't mean to waste your time, but I'm also not responsible for being unaware of conversations that don't involve me. That would be like me trying to hold you accountable when I tweeted that I was implementing this, or when I blogged that this should be part of node:test back in January.

This comment (#48260 (comment)) is the only communication I've seen from you on this topic. First, I don't agree with that opinion. Second, there are already other node:test snapshot libraries on npm. Why should yours appear to be more official based on the package namespace?

@Trott
Copy link
Member

Trott commented May 28, 2024

Dammit. I've been talking with the test runner team about this for like 2 months. Why did you go and implement this in parallel with no discussion or even a heads up to at least warn me you were about to waste all the time I spent on this and its related work.

Hi, Jacob. I know you're surprised and frustrated about this development. I would ask you to do your best to keep things agreeable. (I'm reacting specifically to the "Why did you...waste all the time I spent on this" part. To me, the question reflects an implied accusation. I would probably not have this reaction with "I would have expected this implementation to have been discussed with the test runner team. I'm frustrated that that didn't happen and I feel like all the time I've spent on this was wasted." That puts the emphasis on your expectations and experiences. Anyway, I want to avoid being more of a long-winded nit-picker than I already am, so I'll refrain from explaining my viewpoint in more detail than I already have. Thanks for reading.)

@JakobJingleheimer
Copy link
Contributor Author

JakobJingleheimer commented May 28, 2024

My exasperation comes from having discussed this with the test runner team (I requested it and was then asked to implement it), coordinating with the tooling team (I'm not sure what their official name is, whoever owns those npm namespaces), and (initially) discussing it at the London collab summit. At least a dozen core collaborators knew I was working on this. It's the most recent discussions in the test runner team's official slack channel (which is regularly used), and on multiple test runner GH issues.

My frustration arises because this is highly distributed group, so coordination is paramount. I feel I did just about everything in my power to make everyone aware I was working on this.

Before I start working on anything, I check with the relevant team(s) to see if anyone is already working on it (or has thoughts or wants to collab, or is working on something that might conflict). A distributed team member who neglects to do this is all but guaranteed to create this problem again and again. Which is what appears to have happened here. One thing my node team have done is created a project/issue tracker so anyone can see what is in-flight or needs doing. It's generally worked well.

Regarding why "my" library would be official instead of an existing one on npm: AFAIK, none of those on npm work with node's test runner. Why would you then re-implement it in Core? (So that suggests to me there isn't). "My" library would be official because it's authored and maintained by core collaborators.

The only reason I started on this trek was because it's a use-case I felt critical to the "how to use" guide I'm writing on node's test runner (which I coordinated with Claudio et al who need people to write guides).

Guide needs snapshotting

snapshotting for test runner doesn't exist
→ collab w relevant teams on the design, how and where to publish (core vs @pckgjs vs @nodejs)
→ snapshotting isn't properly possible because it needs the props from this PR
→ sync with the test runner team about adding them.

My frustration here is a bit of an accumulation of similar occurrences where I get like 90-95% done with something (after weeks or months of collab and coord) and get steamrollered by someone who did not coordinate. Once was by a newcomer, which is fully excusable (and his implementation was better than mine), but from Core Collaborators, I feel this is not acceptable. This situation seems to have been trivially easy to avoid, which is perhaps the most frustrating.

EDIT: I didn't even particularly want to do this work; I only did it because it needed doing and I was asked to do it. I would have been happy to step aside (or collaborate with Colin on it).

@aduh95
Copy link
Contributor

aduh95 commented May 28, 2024

I feel this is not acceptable

I certainly understand the frustration, but I very much disagree that sending a PR (even out-of-the-blue) can be qualified as not acceptable – after all it’s free work, you having spend time on the same problem should not make Colin’s contribution less valuable. Was your time wasted? You’re the only judge to that, but even if the answer is yes, you cannot blame others for that. There’s certainly value in finding out where the communication failed, to try to get more productive collaboration in the future, but unfortunately I think this problem is inherent to open-source: at any point, someone can come up with an alternative solution, and that’s fair-game.

@JakobJingleheimer
Copy link
Contributor Author

Yes, the problem here is lack of communication. An alternative idea is great! But not in isolation—that is not collaboration, which is what this project is supposed to be.

this problem is inherent to open-source

Absolutely not. This is a problem well solved, and I just described 2 ways that solve it.

@JakobJingleheimer
Copy link
Contributor Author

Anyway, @cjihrig your work is great. I haven't got a chance to fully review your module mocking PR, but the first 70% looks 🤩 And your implementation for snapshotting may well be better than mine.

It seems like the general opinion here is that having huge amounts of my time wasted should be expected and should be acceptable. I have far better things to do than that though, so I will likely not be active on this project for a while until that changes.

@juliangruber
Copy link
Member

My exasperation comes from having discussed this with the test runner team

Where and with who did this discussion happen?

@JakobJingleheimer
Copy link
Contributor Author

Where and with who did this discussion happen?

In the team's slack channel (and possibly other more general channels). IIR, Moshe was involved in all of them.

@MoLow
Copy link
Member

MoLow commented May 28, 2024

For reference, I think this is the discussion on slack (I might be wrong and there are more): https://openjs-foundation.slack.com/archives/C06UJ98LY6R/p1713625929870859
I don't recall a discussion about adding it, just asking if it is feasible (again, I might be wrong).
anyway - to be more constructive, I propose we land this first, then land #53169 on top of it. either that or add @JakobJingleheimer as a co-author of d5b7927?
@cjihrig @JakobJingleheimer are you comfortable with any of these suggestions? do you prefer one over another?

Copy link
Member

@MoLow MoLow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes LGTM, can you add documentation & fix linter?

@cjihrig
Copy link
Contributor

cjihrig commented May 28, 2024

I'm fine with adding a co-author to d5b7927. I have some concerns with landing this PR.

  • It has a bug. file() will crash in some cases.
  • If file() lands, I think it would be better to change it to location() and return a copy of the whole location object instead of just the file.
  • While I'm not opposed to adding file()/location(), I'm not sure that it is needed in order to implement snapshotting. I think it will lead to snapshot implementations potentially overwriting their own files in certain cases. (I say this without having seen the implementation using it)
  • There are no docs.
  • The test seems a bit convoluted.

@JakobJingleheimer JakobJingleheimer deleted the test_runner/expose-props-for-snapshots branch May 30, 2024 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants