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

type stripping + module mocking causes test runner to throw #54428

Closed
JakobJingleheimer opened this issue Aug 17, 2024 · 6 comments · Fixed by #54878
Closed

type stripping + module mocking causes test runner to throw #54428

JakobJingleheimer opened this issue Aug 17, 2024 · 6 comments · Fixed by #54878
Labels
strip-types Issues or PRs related to strip-types support test_runner Issues and PRs related to the test runner subsystem.

Comments

@JakobJingleheimer
Copy link
Member

Version

22.6.0

Platform

Irrelevent

Subsystem

module

What steps will reproduce the bug?

  1. Create a typescript file (ex logger.ts)
  2. Create a test file (ex replace-js-ext-with-ts-ext.test.ts)
  3. Within the test file, set up a module mock
    const logger = mock.fn();
    
    before(async () => {
      mock.module('./logger.ts', {
        namedExports: { logger }
      });
    
      await import('./replace-js-ext-with-ts-ext.ts');
    });
  4. Run the test (ex node --experimental-test-module-mocks --experimental-strip-types --test ./replace-js-ext-with-ts-ext.test.ts)

How often does it reproduce? Is there a required condition?

100%

What is the expected behavior? Why is that the expected behavior?

It should behave the same as when the module mock target is a node builtin or a javascript file.

What do you see instead?

  TypeError [ERR_INVALID_ARG_VALUE]: The argument 'format' must be one of: 'builtin', 'commonjs', 'module'. Received 'module-typescript'
      at MockTracker.module (node:internal/test_runner/mock/mock:517:7)
      at SuiteContext.<anonymous> (file:///[…]/JakobJingleheimer/correct-ts-specifiers/replace-js-ext-with-ts-ext.test.ts:37:23)
      at TestHook.runInAsyncScope (node:async_hooks:206:9)
      at TestHook.run (node:internal/test_runner/test:865:25)
      at TestHook.run (node:internal/test_runner/test:1116:18)
      at TestHook.run (node:internal/util:545:20)
      at node:internal/test_runner/test:785:20
      at async Suite.runHook (node:internal/test_runner/test:783:7)
      at async Suite.run (node:internal/test_runner/test:1220:7)
      at async Test.processPendingSubtests (node:internal/test_runner/test:574:7) {
    code: 'ERR_INVALID_ARG_VALUE'
  }

Additional information

No response

@avivkeller avivkeller added test_runner Issues and PRs related to the test runner subsystem. strip-types Issues or PRs related to strip-types support labels Aug 17, 2024
@avivkeller
Copy link
Member

I can't reproduce:

import { before, mock } from 'node:test';

const logger = mock.fn();

before(async () => {
  mock.module('./logger.ts', {
    namedExports: { logger }
  });

  await import('./replace-js-ext-with-ts-ext.ts');
});
(node:745974) ExperimentalWarning: Type Stripping is an experimental feature and might change at any time
(Use `node --trace-warnings ...` to show where the warning was created)
(node:745980) ExperimentalWarning: Type Stripping is an experimental feature and might change at any time
(Use `node --trace-warnings ...` to show where the warning was created)
(node:745980) [MODULE_TYPELESS_PACKAGE_JSON] Warning: file:///index.test.ts parsed as an ES module because module syntax was detected; to avoid the performance penalty of syntax detection, add "type": "module" to /package.json
(node:745980) ExperimentalWarning: Module mocking is an experimental feature and might change at any time
✔ index.test.ts (93.695635ms)
ℹ tests 1
ℹ suites 0
ℹ pass 1
ℹ fail 0
ℹ cancelled 0
ℹ skipped 0
ℹ todo 0
ℹ duration_ms 100.76966

JakobJingleheimer added a commit to JakobJingleheimer/correct-ts-specifiers that referenced this issue Aug 17, 2024
@JakobJingleheimer
Copy link
Member Author

You can see it here: JakobJingleheimer/correct-ts-specifiers@6deda58

Just change the file extension of logger back to .ts and run npm test. It succeeds as .js and fails as .ts.

@kruncher
Copy link

I am seeing the same issue when experimenting with these two flags.

@marco-ippolito
Copy link
Member

marco-ippolito commented Aug 27, 2024

I cannot reproduce on main, can you check?
I can actually

@cjihrig
Copy link
Contributor

cjihrig commented Aug 27, 2024

I haven't verified this issue, but I would expect it to be valid since the module-typescript (and others) format was introduced, and the module mocking code explicitly checks for one of these formats.

It might be that it's tricky to reproduce when ambiguous files are used?

nodejs-github-bot pushed a commit that referenced this issue Sep 19, 2024
PR-URL: #54878
Fixes: #54428
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Erick Wendel <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
targos pushed a commit that referenced this issue Oct 4, 2024
PR-URL: #54878
Fixes: #54428
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Erick Wendel <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
louwers pushed a commit to louwers/node that referenced this issue Nov 2, 2024
PR-URL: nodejs#54878
Fixes: nodejs#54428
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Erick Wendel <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
tpoisseau pushed a commit to tpoisseau/node that referenced this issue Nov 21, 2024
PR-URL: nodejs#54878
Fixes: nodejs#54428
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Erick Wendel <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
@jehon
Copy link

jehon commented Nov 29, 2024

Note for those coming here:

This worked for me:

node --experimental-strip-types --experimental-transform-types --test "$test"

But --test must come after the experimental flags the end. Order is important.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
strip-types Issues or PRs related to strip-types support test_runner Issues and PRs related to the test runner subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants