connor4312
test file discovery should match Node.js test runner
The extension should discover test files, the way Node.js would discover them.
While there are open issues about discovery of dynamic tests, this is different from those.
When it comes to test files, the Node.js test runner only cares about matching globs (and provided options). The extension seems to deviate from that, or have some kind of bug. I'm getting mixed results (based on whether I "reload window") and in any case, the file list does not match Node.js.
Unfortunately, the mentioned inconsistencies make it hard to reproduce what the extension discovers on my end. So far I'm under the impression that tests in test folders are discovered fairly consistently, while tests matching one of the file extension patterns sometimes show up and sometimes they don't. I haven't tested custom patterns or file name patterns other than the mentioned secondary extensions (e.g. patterns like .test.js were tested and patterns like _test.js were not).
Node.js simply does globSync from cwd against the patterns listed in the docs (or user supplied patterns), always excluding node_modules. The list is created this way even in watch mode and updates are applied on a diff against the previous file list (for the Node.js test runner that would be reruns on newly added [or modified] files only; for this extension deleted files would have to be taken into account as well, to remove them in the GUI).
Any chance one could simply get what Node.js does by default, at least for file discovery? This should of course include all file extensions that Node.js would discover by default as well.
# runs all tests (excluding those in `node_modules`)
node --test
Use case: The command in the example allows for running all tests, even across workspaces in a mono repo, regardless of package manager and mono repo structure or programming language (as far as supported by Node.js).
Without file discovery reflecting Node.js this extension kind of misses its purpose and becomes more or less useless.
This extension uses the same globs that Node.js does. Can you be more specific about the issue(s) you're seeing?
As mentioned, I'm simply not getting the same files listed, and unfortunately I didn't get consistent results in the past. Right now it seems to be a stable miss match in one of my repos: 11 files listed in the extension versus 28 discovered by Node.js.
Aside: The total number of tests is significantly different, since as of now the extension is known not to discover dynamic tests, but I made sure not to include those by accident.
In principle, I don't see any obvious pattern about why a file is being listed or not.
After looking into some of those files, I noticed some of them use the node:test module, and some of them don't. When adding an import and test("name") call to one of the test files currently not using this module, the module is added to the tree. This makes me think, or hope, to have pinpointed it down.
However this still would be deviating from Node.js. Test files don't have to use this module to be considered a test. Any valid script or module will do, as long as it matches the file pattern.
Also: No it doesn't. It's missing TypeScript extensions, but there's already at least one issue open on that.
After looking into some of those files, I noticed some of them use the node:test module, and some of them don't. When adding an import and test("name") call to one of the test files currently not using this module, the module is added to the tree
Yes, we do require node:test to be imported. Otherwise there are no tests in the file which we can discover. I would accept a PR add a config option to change this behavior such that files without tests are included, but would not want to do it eagerly because then it is very likely our test discovery ends up being used in workspaces where node:test isn't the intended runner, as the file patterns are quite generic between any number of other testing frameworks.
As mentioned, I'm simply not getting the same files listed, and unfortunately I didn't get consistent results in the past. Right now it seems to be a stable miss match in one of my repos: 11 files listed in the extension versus 28 discovered by Node.js.
Can you please be specific about the file tree you have / the files you see are or aren't included?
There's a section of documentation on the execution model and several additional paragraphs on useful information on this issue. The following block quote is from the docs:
Matching files are executed as test files. [...]
Test runner execution model #
When process-level test isolation is enabled, each matching test file is executed in a separate child process. [...] If the child process finishes with an exit code of 0, the test is considered passing. Otherwise, the test is considered to be a failure. Test files must be executable by Node.js, but are not required to use the node:test module internally.
Each test file is executed as if it was a regular script. That is, if the test file itself uses node:test to define tests, all of those tests will be executed within a single application thread, regardless of the value of the concurrency option of test().
When process-level test isolation is disabled, each matching test file is imported into the test runner process. Once all test files have been loaded, the top level tests are executed with a concurrency of one. Because the test files are all run within the same context, it is possible for tests to interact with each other in ways that are not possible when isolation is enabled. For example, if a test relies on global state, it is possible for that state to be modified by a test originating from another file.
To get back to this thread:
Yes, we do require node:test to be imported.
Every test file not explicitly using the test module is implicitly using (wrapped / executed as) a default Test instance. According to the execution model, that is a feature. And an ad hoc algorithm that filters test files based on some sort of static analysis works against that feature.
Configurable filtering of tests is done via file lists, file globs and test name patterns - and a couple of more options like only, but none of those additional options influence static file discovery. Further more, files can be added and removed dynamically via the watch option. All of this already addresses your concerns about the following:
[...] but would not want to do it eagerly because then it is very likely our test discovery ends up being used in workspaces where node:test isn't the intended runner, as the file patterns are quite generic between any number of other testing frameworks.
Yes, test file naming conventions and directory structures are consistent across testing frameworks, and that's a good thing. All of the mentioned file name or pattern options are precisely the already builtin features which give you control over this. Most importantly, the reason your test discovery makes it likely to fail is, because you remove access to those features and enforce your own patterns (being a subset of hard to configure file extensions), buried somewhere in the extension's IDE settings, rather than keeping it a part of the tool's interface.
Additionally, Node.js discovers subtests based on runtime instances and actual test calls. Again, zero static analysis is involved in this, beyond file discovery. Tests can even be invoked lazily, by just running a script without the --test or run() interfaces. This further shows, that all that's needed in Node.js to be considered a test, is an arbitrary entry point to start the engine (assuming the process survives until it reaches the test call).
Rerunning tests is also already built in and works on a file by file basis and configuration options and on the state that the runner captures for you. The simplest example would be --watch options, but there's also --test-rerun-failures, and maybe more. The layout of state files for reruns (on failure) is documented as well. Once more, it does not work by static code analysis.
The docs are also clear about that tooling building on top of the test runner should use <TestStream>, given by invoking run().
What one should expect from an IDE/GUI integration: A nice graphical representation of what is already possible with run() and/or node --test, following their official APIs and documentation, starting from runner options/arguments down to reporters. E.g. the focus should be on making configurations (and lists, trees, function calls and so on) pleasingly workable in terms of typography, visuals, organization and interaction beyond what's possible in a terminal.
I would accept a PR add a config option to change this behavior
Yes, I'm considereing doing so, given that the goal of this extension is to provide a GUI for working with the Node.js. test runner. I won't be considering it, if the following is the case: that this extension is just named in a way that sounds as if it uses the test runner but (by design, not by accident) puts its own static analysis over the test discovery algorithm implemented by Node.js. If the latter is the case, then maybe it would be better to look for (or build) another extension. Just let me know.
Can you please be specific about the file tree you have / the files you see are or aren't included?
I already was, or I thought so. What additional information apart from what I wrote would be helpful? Given your confirmation that node:test imports are indeed a discovery requirement in the extension, I don't think there's information missing above what was mentioned:
After looking into some of those files, I noticed some of them use the node:test module, and some of them don't. When adding an import and test("name") call to one of the test files currently not using this module, the module is added to the tree. This makes me think, or hope, to have pinpointed it down.