implement test specifier configuration by skabbes · Pull Request #64 · connor4312/nodejs-testing

skabbes · 2025-03-27T22:44:30Z

Original Discussion here:
#63

skabbes · 2025-03-27T22:46:10Z

src/controller.ts

-    if (!contents.includes("node:test")) {
-      this.deleteFileTests(uri.toString());
-      return;
-    }


Unfortunately, this is no longer a good cheap relevancy test - but if you'd like, I'd be happy to update it to be:

"If your test specifiers only reference packages, and not relative imports - then we can construct a cheap relevancy test for those packages".

We could match the extensionless basename of any TestFunctionSpecifierConfig.name. Less specific but still cheaper than parsing the AST unnecessarily.

skabbes · 2025-03-27T22:47:16Z

src/controller.ts

    }

-    const tree = parseSource(contents);
+    const tree = parseSource(contents, folder, uri, this.testSpecifiers);


Kind of a gross API, but I wanted to keep the code structure as similar as possible without getting your feedback. Making these parameters optional meant most of the tests didn't need change.

skabbes · 2025-03-27T22:47:33Z

src/controller.ts

-    watcher.onDidCreate((uri) => this.includeTest(uri.fsPath) && this._syncFile(uri));
-    watcher.onDidChange((uri) => this.includeTest(uri.fsPath) && this._syncFile(uri));
+    watcher.onDidCreate((uri) => this.syncFile(uri));
+    watcher.onDidChange((uri) => this.syncFile(uri));


I believe this is what syncFile was doing anyway.

skabbes · 2025-03-27T22:48:50Z

src/test-function-specifier-config.ts

+ */
+export interface TestFunctionSpecifierConfig {
+  /** The names of the functions that should be included in the test runner view */
+  name: string[] | string;


Maybe this should always be an array - I didn't realize the JSON schema in the package.json was going to be awkward to express this union type.

src/utils.ts

connor4312

Looks generally sane, some comments!

connor4312 · 2025-03-28T04:48:20Z

src/extension.ts

    },
  ]);

+  const testSpecifiers = new ConfigValue("testSpecifiers", defaultTestFunctionSpecifiers);


Make sure to register an onChange listener for this down below, like we do for includePatterns

connor4312 · 2025-03-28T04:52:14Z

src/controller.ts

    include: string[],
    exclude: string[],
    extensionConfigs: ExtensionConfig[],
+    testSpecifiers: TestFunctionSpecifierConfig[],


Suggested change

testSpecifiers: TestFunctionSpecifierConfig[],

private readonly testSpecifiers: TestFunctionSpecifierConfig[],

Oh interesting, I didn't know about this little syntactic sugar.

connor4312 · 2025-03-28T04:53:28Z

src/controller.ts

+   * @param contents the file contents of uri - to be used as an optimization
+   */
  private async _syncFile(uri: vscode.Uri, contents?: string) {
+    const folder = vscode.workspace.getWorkspaceFolder(uri);


The Controller is already rooted at a particular workspace folder. We shouldn't have any URIs passed here that aren't in that workspace folder (if so that's a bug)

ok, perfect - I'll update.

connor4312 · 2025-03-28T04:54:29Z

src/controller.ts

-    if (!contents.includes("node:test")) {
-      this.deleteFileTests(uri.toString());
-      return;
-    }


We could match the extensionless basename of any TestFunctionSpecifierConfig.name. Less specific but still cheaper than parsing the AST unnecessarily.

connor4312 · 2025-03-28T04:57:47Z

src/parsing.ts

+
+  let importValue = node.source.value;
+  if (node.source.value.startsWith("./") || node.source.value.startsWith("../")) {
+    if (!folder || !fileUri) {


I think neither of these should ever be undefined here. We only parse from Controllers rooted in a workspace folder, and only parse files once saved on disk.

That is correct - but since I left them optional, I was just making Typescript happy until I had your feedback on what to do for this API. I'll make them required and remove these superfluous checks.

connor4312 · 2025-03-28T04:58:56Z

src/parsing.ts

+      Path.resolve(Path.dirname(fileUri.fsPath), node.source.value),
+    );
+
+    importValue = `./${importRelativeToRoot}`;


We need to normalize to forward slashes on Windows

Good catch, but hmm. I think the issue here is that I am moving from a URI "path" (which would be the same on all platforms) to a filesystem path.

If the logic stayed as a URI, then I don't think we'd need to worry about that. Let me see if I can re-work this logic and avoid switching to filesystem paths, so we can avoid the cross platform issues.

connor4312 · 2025-03-28T05:02:54Z

src/parsing.ts

+    for (const spec of node.specifiers) {
+      const specType = spec.type;
+      if (specType === C.ImportDefaultSpecifier || specType === C.ImportNamespaceSpecifier) {
+        if (validNames.has("default")) {


The filter shouldn't happen at this level. This covers cases like import * as t from 'node:test'. We should then match validNames on accesses like t.test or t.suite (where the lower branch covers the cases like import { test } from 'node:test)

The unit tests in parsing.test.ts can help you. Run with npx vitest

I think I didn't understand this comment fully until now. But I believe this is solved now.

connor4312 · 2025-03-28T05:03:31Z

src/parsing.ts

+  text: string,
+  folder?: vscode.WorkspaceFolder,
+  fileUri?: vscode.Uri,
+  testFunctions?: TestFunctionSpecifierConfig[],


This should always by defined, the ConfigValue will provide your default value. Just update unit tests :)

skabbes · 2025-03-28T06:25:45Z

@connor4312 thank you for the review - quite a few of your comment were related to some of the API choices I had to make because I was struggling to get the unit tests working. Let me see if I can elaborate a bit more to explain the situation, and see if you happen to know a solution.

Firstly: I am able to run the existing unit tests just fine. No issues at all.

When I was trying to write new unit tests - which I do need a lot of - I hit a snag... I need the tests to pass a fileUri: vscode.Uri and folder: vscode.WorkspaceFolder. It appears that the vscode module is magically provided when you run the extension as an extension. However, in the unit tests - the vscode module is not available. If I try to install the vscode library - I see it is deprecated https://www.npmjs.com/package/vscode and doesn't work anyway. If you know a way to write those unit test in a way that I can access the vscode module, I'm all ears. I thought about just duck-typing it to make Typescript happy - but vscode.Uri is an actual class. Maybe I can define a narrower interface that vscode.Uri satisfies, but I'm starting to get down a weird rabbit hole following that logic.

Alternatively
I can refactor the API of parseSource to not rely on anything from the vscode module, so that I can unit test it (for example, just pass a string for fileUri). I think after trying this for a bit, this is likely my plan of attack for now.

I'm off for the weekend, but will pick this back up next week Monday.

connor4312 · 2025-03-28T17:10:53Z

When I was trying to write new unit tests - which I do need a lot of - I hit a snag... I need the tests to pass a fileUri: vscode.Uri and folder: vscode.WorkspaceFolder. It appears that the vscode module is magically provided when you run the extension as an extension.

Yea, this is something that's injected in the vscode extension host but not Node unit tests. I would just stub out the relevant bits. I think the only complex part is URIs, and for that you can add vscode-uri as a dev dependency and use that to construct the URIs for the file and workspace folder.

Or you can just pass in the .fsPath's for the workspace folder and file in both cases and take strings in parser.ts. I'm good with either :)

skabbes · 2025-03-28T17:15:53Z

OK perfect - I'll make that change and significantly beef up the parsing unit tests, and fix the inevitable issues that arise. Have a great weekend!

This allows unit test to be written and run without VS code.

skabbes

OK, I believe I have made all of the requested changes.

A few notes...
I checked vscode-uri, and noticed that they were just essentially using node:path/posix so used that instead, and stopped using fsPath completely with the URIs.

I implemented the cheap "might have tests" check in exactly the way you suggested (see fileMightHaveTests).

connor4312 · 2025-04-02T17:13:04Z

Nice! I think the last thing is that matchNamespaced should take the set of validNames so it avoid matching any bar in import * as foo from 'x'; foo.bar() which is not one of the valid test names. Currently I think it still follows the old behavior of treating any function from that module as being a test/suite.

skabbes · 2025-04-02T17:17:23Z

Thanks! I'll add a negative test for that and update now.

skabbes · 2025-04-02T20:05:58Z

Thank you for all the help, and for merging, if you could cut a new release at your convenience - I'd be very grateful!

connor4312 · 2025-04-02T20:41:15Z

Published in 1.7.0, thanks for the PR!

implement test specifier configuration

e86e380

skabbes commented Mar 27, 2025

View reviewed changes

skabbes mentioned this pull request Mar 27, 2025

Discussion: allow extension to function when node:test is wrapped by a convenience function #63

Closed

connor4312 reviewed Mar 28, 2025

View reviewed changes

skabbes marked this pull request as draft March 28, 2025 17:16

skabbes added 5 commits March 31, 2025 14:06

implement a cheap check finding tests in files

b7b0e17

parsing: refactor parseSource to not rely on any vscode.* code

293b3ba

This allows unit test to be written and run without VS code.

implement logic for require with custom test specifiers

11d773b

self code review

dde896f

clarify documentation for configuration value

70c00ce

skabbes commented Apr 2, 2025

View reviewed changes

skabbes requested a review from connor4312 April 2, 2025 06:11

skabbes marked this pull request as ready for review April 2, 2025 06:11

Check tests have valid names when using 'namespaced' imports

1099deb

connor4312 approved these changes Apr 2, 2025

View reviewed changes

connor4312 merged commit f0aae88 into connor4312:main Apr 2, 2025
2 checks passed

skabbes deleted the test_specifiers_for_wrapped_test_fn branch April 2, 2025 20:07

	testSpecifiers: TestFunctionSpecifierConfig[],
	private readonly testSpecifiers: TestFunctionSpecifierConfig[],

Comments

Conversation

skabbes commented Mar 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

connor4312 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skabbes commented Mar 28, 2025

Uh oh!

connor4312 commented Mar 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

skabbes commented Mar 28, 2025

Uh oh!

skabbes left a comment

Choose a reason for hiding this comment

Uh oh!

connor4312 commented Apr 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

skabbes commented Apr 2, 2025

Uh oh!

Uh oh!

skabbes commented Apr 2, 2025

Uh oh!

connor4312 commented Apr 2, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

skabbes commented Mar 27, 2025 •

edited

Loading

connor4312 commented Mar 28, 2025 •

edited

Loading

connor4312 commented Apr 2, 2025 •

edited

Loading