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.

Sign up for GitHub

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

Jump to bottom

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

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

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

JakobJingleheimer opened this issue Aug 17, 2024 · 5 comments · Fixed by #54878
Labels
repro-exists Issues with reproductions. 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
Contributor

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
@RedYetiDev RedYetiDev 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
@RedYetiDev
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
tmp: convert logger.ts → logger.js
6deda58 
due to nodejs/node#54428
@JakobJingleheimer
Copy link
Contributor 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.

@fisker fisker mentioned this issue Aug 23, 2024
Open
4 tasks
@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 •
edited
Loading

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?

@RedYetiDev RedYetiDev added the repro-exists Issues with reproductions. label Aug 29, 2024
@marco-ippolito marco-ippolito mentioned this issue Sep 10, 2024
Merged
nodejs-github-bot pushed a commit that referenced this issue Sep 19, 2024
@marco-ippolito
test_runner: support typescript module mocking
bb40521 
PR-URL: #54878
Fixes: #54428
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Erick Wendel <erick.workspace@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
targos pushed a commit that referenced this issue Oct 4, 2024
@marco-ippolito @targos
test_runner: support typescript module mocking
93c6c90 
PR-URL: #54878
Fixes: #54428
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Erick Wendel <erick.workspace@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
louwers pushed a commit to louwers/node that referenced this issue Nov 2, 2024
@marco-ippolito @louwers
test_runner: support typescript module mocking
8b8a4b2 
PR-URL: nodejs#54878
Fixes: nodejs#54428
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Erick Wendel <erick.workspace@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
repro-exists Issues with reproductions. strip-types Issues or PRs related to strip-types support test_runner Issues and PRs related to the test runner subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

test_runner: support typescript module mocking marco-ippolito/node
5 participants
@kruncher @cjihrig @JakobJingleheimer @marco-ippolito @RedYetiDev