improve the structure of the test-event-emitter-num-args file and use the node test runner

[dario-piotrowicz]

this PR includes a very minor refactoring of the test-event-emitter-num-args file, mostly for improving readability and avoiding the pattern of collecting values and assert them all in one go

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.20%. Comparing base (da5f7ac) to head (c8a4a77).
Report is 13 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #56671      +/-   ##
==========================================
- Coverage   89.21%   89.20%   -0.01%     
==========================================
  Files         662      662              
  Lines      191893   191934      +41     
  Branches    36936    36948      +12     
==========================================
+ Hits       171189   171222      +33     
- Misses      13541    13542       +1     
- Partials     7163     7170       +7     

see 25 files with indirect coverage changes

[lpinca]

In my opinion it increases the mental overhead and reduces readability instead of improving it.

[dario-piotrowicz]

Hey @lpinca 🙂, thanks for having a look

Is there something specific that you find reduces readability and increases mental overhead that I maybe could improve on?

[lpinca]

1. I don't think node:test adds any benefits here. It is only spurious code that is run as part of the test that only makes the test run slower. You can read my opinion on the subject here Discussion/Tracking: Adding more structure to Node.js' tests #54796, doc: add restrictions around node:test usage #56027, test: improve zlib tests #55716, and other linked comments.
2. Destructuring, for loops, why? The original is so much simpler.

[dario-piotrowicz]

1. I don't think node:test adds any benefits here. It is only spurious code that is run as part of the test that only makes the test run slower. You can read my opinion on the subject here Discussion/Tracking: Adding more structure to Node.js' tests #54796, doc: add restrictions around node:test usage #56027, test: improve zlib tests #55716, and other linked comments.

That's super interesting, thanks a lot! I'll have a good read! Naively I thought that node:test is simply a better structured way to run the tests, didn't imagine that there'd be some (non-negligible) perf downsides to that

2. Destructuring, for loop, why? The original is so much simpler.

Yeah I agree with that one, initially I had something like [0, 1, 2, 3, 4, 5] there, that would be simpler to read (although the destructuring allows us to test more number of arguments)

[jasnell]

Adding to @nodejs/tsc agenda because I think we need a TSC ruling on the debate or whether to restructure these tests around node:test so we don't have to fight the battle file by file.

[lpinca]

It is already on the TSC agenda (see #55716 and #56027), and I did not block when it was an actual improvement (#56562, #56437, #56439). Also node:test is only half of the problem here, and in #56027 the proposal is to not use node:test for node:events tests.

[cjihrig]

FWIW, I think the original test is more readable here. Not because of node:test, but because of some of the other rewrites here.

There are other tests which I think can benefit from this type of refactor more. Specifically, tests that spawn multiple child processes at the same time. I recently performed a couple of those updates in 93c15bf and 7e08cca.

Other candidates are things that the test runner already gives you out of the box such as a test runner (221040b) or mocking. I also think we could remove a lot of the .then(common.mustCall()); pattern (test/parallel/test-blob-file-backed.js and I think webcrypto tests do that a lot and may be of interest to @jasnell as the author).

[jasnell]

Taking this back off the tsc-agenda.

[joyeecheung]

tests that spawn multiple child processes at the same time.

FWIW for this kind of cases, I think it would be better not to spawn multiple child processes in one file at all if it's possible. It's better to simply split them into one file per test case, which also helps isolating the tests (and when you only have one test case per file it's generally unnecessary to use node:test to group them, it's mostly useful only for mocks).

When we squeeze too many test cases in one file, when one break only part of them locally one has to look for the cause of failure in a huge sea of logs that do not fit in the screen scroll limit (grouping them with node:test is especially not good for this as it outputs too many useless information, and if you put logs inside the broken test case you can't even see them in this sea, try breaking the module loader and run test/es-module/test-esm-loader-hooks.mjs and you'll see what I mean), and when it flakes due to e.g. deadlock in the CI you have no idea which one of the test case is flaking.

[cjihrig]

You are entitled to your opinion, but please stop conflating people writing bad tests with people using a test runner. Every point you bring up about these tests has happened before the test runner existed and will continue to exist, including stuffing too many tests in a single file via block scopes or IIFEs.

Quite frankly, it's extremely annoying, particularly when people have made efforts (0576deb, afaa14b) to improve the situation for people like you.

[jasnell]

@cjihrig ... who are you directing your comments to?

[cjihrig]

The comment immediately preceding mine.

[aduh95]

On a related note, we have introduced an env variable to help with setting the optimal concurrency in #52177 – arguably using node:test helps with that, it wouldn't make sense to reimplement the concurrency feature.

[joyeecheung]

please stop conflating people writing bad tests with people using a test runner

I am not sure if I was clear enough, to clarify, I was trying to say that there were bad tests and while it was not limited to node:test, the use of node:test made the problem worse.

Every point you bring up about these tests has happened before the test runner existed and will continue to exist, including stuffing too many tests in a single file via block scopes or IIFEs.

While this is true, I think node:test made the situation worse in two aspects:

1. Its "catch-and-continue" makes the output of these tests even harder to read. Previously if you just break one case in a test that "stuff too many tests in a single file via blocs or IIFEs", locally you can focus on that one, fix it and repeat. Now you need to look at a sea of logs.
2. If we encourage PRs motivated only by the idea of using node:test-based grouping more, then it is already giving in to the idea of "it's fine to group multiple tests cases in one file more" instead of "how about just don't group at all and split them into different files?". If we recommend isolating the tests instead, node:test just would not come into the picture most of the times because it would look like an overkill to use any of describe/test etc. if there's only one single test case in one file.

...to improve the situation for people like you

Not sure who I am being conflated with, but the issues you referenced seems to be caused by the practices I advise against here. test/es-module/test-esm-loader-hooks.mjs is one of the tests where I had to follow other people's decisions of doing tests this way even though I did not it like because it made debugging harder for me, and made me realize the problems mentioned above. The pattern of test-esm-loader-hooks.mjs is pretty contagious in test/es-module and I had to push back explicitly when I added tests that only test fewer things per file, which is why I came to the conclusion of 2. I don't think I would've needed to argue that "it's better not to group them in one file at all" if we didn't need to consider describe()/test(), as it would seem more obvious that having no blocks or IIFEs due to the file-based isolation is better than having a bunch of them in one file.

I'm just advocating that instead of migrating existing tests blindly with node:test with all these problem unsolved, which also put pressure on others to fix it at the meantime (which may or may not happen), it's better to just keep things as-is and only adopt node:test in places when we know how to minimize these problems/some workarounds are already in place to help with them. This ordering would also minimize the pressure on you.

[joyeecheung]

arguably using node:test helps with that, it wouldn't make sense to reimplement the concurrency feature.

This doesn't really address the debuggability problem, or it makes things worse because it continues to encourage grouping test cases in one file. On the other hand, the python test runner is already capable of running the tests in parallel. If we just isolate the each test case that spawn child processes into their own files, not only does it help with debuggability, we also get parallelism without the use of node:test.

[lpinca]

@cjihrig it was/is also frustrating/annoying seeing PRs being merged with the complicity of the node:test proposers (#54574, #54582, #54609, etc.) when concerns were being raised and discussed until I started blocking those.

If you as TSC members honestly think this PR and the others I have blocked are good for the project and a mass refactor is needed, then I think I no longer align with the project values, priorities, and vision, and I'll think about stepping down as a core contributor.

[cjihrig]

it was/is frustrating seeing PRs being merged

I understand that. I am personally not advocating for a mass rewrite of all of our tests. I also think it is fine for some people to prefer using node:test and other people to not want to use it.

I just don't appreciate the framing of "Node's tests were great and this is ruining them." We have good and bad tests written using node:test, and we have good and bad tests written without it.

[jasnell]

@dario-piotrowicz .... My apologies. I know you're a brand new contributor seeking to learn how to contribute to the project and that discussions like what you're seeing here among existing core contributors is both not helpful and actively discouraging of new contributions, and for that I apologize. There are other areas of the codebase that would likely be easier to contribute to and I'll help you identify those. In the meantime I would recommend just abandoning this PR.

[joyeecheung]

I also want to apologize for hijacking the thread a bit to discuss about node:test per-se. To explain a bit, in general it is riskier to submit a pull request that only changes code for stylistic reasons that are not part of the style guide, because stylistic preferences are subjective. Node.js is a project that operates on consensus among the collaborators, so changing one style to another needs consensus about which one is better. In this case, we have no consensus about whether the use of node:test improves the tests, and its merits in the built-in test suite are still under debate, within the context that Node.js itself has been using a Python-based test runner to run the tests, and the majority of the tests were written with that in mind instead, while node:test is a recent addition and it will take time before we find the consensus on how to fit node:test into the existing test suite or use it together with the existing test running infrastructure.

[dario-piotrowicz]

@dario-piotrowicz .... My apologies. I know you're a brand new contributor seeking to learn how to contribute to the
...

No need to apologize, thanks for backing my changes up 😊

Hopefully I'll have better luck with my next contribution attempt 😄🤞

[dario-piotrowicz]

I also want to apologize for hijacking the thread a bit to discuss about node:test per-se. To explain a bit, in general it is
...

Thanks! 🫶

Again no need to apologize, I naively thought using node:test (also in the node project itself) was a no-brainer, but I was clearly wrong, and only after I opened the PR I realized how much story this has 😅

Anyways thanks a lot for the extra context! is very much appreciated 🙂

[dario-piotrowicz]

Closing the issue as per the above conversation, thanks y'all for having a look, I hope I didn't waste anyone too much time here 👋