test: skip failing tests and set the default value of strip-types to false when compiled without amaro #60815

y-okt · 2025-11-22T15:43:38Z

Summary

This PR is to address #60640 . When compiled without amaro, some tests fail. This is because --strip-types option should default to false when compiled without amaro, but it is set to true now. As suggested by @marco-ippolito , this PR addresses this issue by

skipping failing tests
set the default value of --strip-types to false if HAVE_AMARO is not true

Background

Amaro is a wrapper around @swc/wasm-typescript, and is used internally in Node.js for Type Stripping. When compiled without amaro, Type Stripping functionalities are not available internally. However, because --strip-types default value is true, some tests fail under that situation.
The suggested fix is as aforementioned, 1) skip the failing tests when the option is false, and set its default value to false when compiled without amaro.

Regarding the failing tests, the list below is the full list of the failing tests due to Amaro. When running with amaro, all the tests passed, and when running without amaro, the tests below failed, meaning that test failures are attributed to the existence of amaro.
Also, I confirmed that these tests are failing not due to flakiness, but amaro. Most of the failures were with the Error [ERR_NO_TYPESCRIPT]: Node.js is not compiled with TypeScript support error message. For the files which don't have this error message, the reason it's relevant to amaro is written below respectively.

List of failing tests when compiled without amaro:

test/parallel/test-compile-cache-typescript-commonjs.js
test/parallel/test-compile-cache-typescript-esm.js
test/parallel/test-compile-cache-typescript-strip-miss.js
test/parallel/test-compile-cache-typescript-transform.js
test/parallel/test-compile-cache-typescript-strip-sourcemaps.js
test/parallel/test-config-file.js
test/parallel/test-node-output-eval.mjs
test/parallel/test-node-output-sourcemaps.mjs
test/parallel/test-runner-coverage-default-exclusion.mjs <- Failed because Node tries to strip types from file.test.ts
test/parallel/test-runner-run-global-hooks.mjs
test/parallel/test-runner-global-setup-teardown.mjs <- Failed because typescript file is read (code)
test/parallel/test-util-getcallsites.js
test/parallel/test-worker-cli-options.js
test/parallel/test-worker-eval-typescript.js
test/parallel/test-worker-load-file-with-extension-other-than-js.js
test/parallel/test-worker-syntax-error.js <- Failed because type stripping is tried (code, code)
test/es-module/test-esm-import-meta-main-eval.mjs
test/es-module/test-esm-detect-ambiguous.mjs
test/es-module/test-esm-tla-syntax-errors-not-recognized-as-tla-error.mjs
test/test-runner/test-output-typescript-coverage.mjs

Note that the environment in which this was confirmed is M4 Mac and OS is Sequoia 15.3.2.

nodejs-github-bot · 2025-11-22T15:43:44Z

Review requested:

@nodejs/config
@nodejs/test_runner

marco-ippolito · 2025-11-22T17:06:05Z

Good job 👍🏼
I dont think we should skip the whole file for:

test/es-module/test-esm-import-meta-main-eval.mjs
test/es-module/test-esm-detect-ambiguous.mjs
test/es-module/test-esm-tla-syntax-errors-not-recognized-as-tla-error.mjs
test/parallel/test-node-output-eval.mjs
test/parallel/test-node-output-sourcemaps.mjs
test/parallel/test-runner-coverage-default-exclusion.mjs <- Failed because Node tries to strip types from file.test.ts
test/parallel/test-runner-run-global-hooks.mjs
test/parallel/test-runner-global-setup-teardown.mjs <- Failed because typescript file is read ([code](https://github.com/nodejs/node/blob/fb6b83c9eface51acad916cff365b98aa826f56a/test/parallel/test-runner-global-setup-teardown.mjs#L265))
test/parallel/test-util-getcallsites.js
test/parallel/test-worker-cli-options.js

We can skip just the failing subtest, you can pass an argument { skip: process.config.variables.node_use_amaro } to the test function if the test is using the node.js test runner

codecov · 2025-11-22T18:20:31Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.53%. Comparing base (1758b74) to head (3dc852d).
⚠️ Report is 6 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #60815      +/-   ##
==========================================
- Coverage   88.56%   88.53%   -0.03%     
==========================================
  Files         703      703              
  Lines      208268   208268              
  Branches    40175    40179       +4     
==========================================
- Hits       184444   184399      -45     
- Misses      15828    15878      +50     
+ Partials     7996     7991       -5

Files with missing lines	Coverage Δ
src/node_options.cc	`77.85% <ø> (+0.10%)`	⬆️

... and 33 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

kapouer · 2025-11-22T19:30:24Z

That's great, I'll be glad to test when it's ready and before it's merged.

src/node_options.cc

test/es-module/test-esm-import-meta-main-eval.mjs

test/es-module/test-esm-tla-syntax-errors-not-recognized-as-tla-error.mjs

src/node_options.cc

When compiled without amaro, this conflicts with --experimental-strip-types defaulting to true. To avoid that, we need to skip relevant failing tests. Fixes: nodejs#60640

stripe-types option's default value is true, but we need to ensure that when compiled without amaro, the default vaue is false. Fixes: nodejs#60640

Move the amaro availability check immediately after importing common module in test files, as requested in PR feedback. Changes: - Place amaro check after common import but before other imports - Update test-node-output-sourcemaps.mjs to dynamically skip .ts tests based on file extension - Add individual skip conditions for TypeScript-specific tests - Fix linting errors (missing semicolon, line length issues) Refs: nodejs#60815

…ne pass fixed the test failure due to its changeable default value. Fixes: nodejs#60640

y-okt · 2025-11-23T14:33:31Z

Thank you very much for your reviews. I addressed the points. Could you please take a look again? Thank you in advance!

y-okt · 2025-11-23T14:36:11Z

test/parallel/test-cli-node-options-docs.js

+  // Exception for HAVE_AMARO conditional default (defaults to true when Amaro is available)
+  if (config.includes('HAVE_AMARO')) {
+    hasTrueAsDefaultValue = true;
+  }
+


This is added because the value of HAVE_AMARO is changeable (the default is true), and this test can't deal with that. Rather than adding a complex logic like checking #if in config (which is not 100% correct because, like in this case, changeable variables such as HAVE_AMARO can be assigned instead of using #if), I added this logic instead

cant we use process.config.variables.node_use_amaro ?

marco-ippolito · 2025-11-23T14:59:38Z

test/es-module/test-esm-detect-ambiguous.mjs

-      assert.strictEqual(code, 1);
-      assert.strictEqual(signal, null);
-    });
+    it('still throws on `await` in an ordinary sync function',


This should not be related to amaro 🤔 why is it failing

Hi, I think this is related to amaro, because eval used in this test calls evalTypeScript (code), which is triggered by --strip-types option (code). Even when compiled without amaro, this means that the defalut value of --strip-types will be false, but the users can manipulate this values by setting true. Thus, as long as TypeScript type stripping is triggered due to --strip-types, skipping this test sounds good to me. What do you think?

This is the error message:

Details

=== release test-esm-detect-ambiguous === Path: es-module/test-esm-detect-ambiguous Test failure: 'still throws on `await` in an ordinary sync function' Location: test/es-module/test-esm-detect-ambiguous.mjs:266:5 AssertionError [ERR_ASSERTION]: The input did not match the regular expression /SyntaxError: await is only valid in async function/. Input:
'node:internal/process/execution:279\n' +
' throw tsError;\n' +
' ^\n' +
'\n' +
'Error [ERR_NO_TYPESCRIPT]: Node.js is not compiled with TypeScript support\n' +
' at assertTypeScript (node:internal/util:244:11)\n' +
' at node:internal/modules/typescript:46:3\n' +
' at node:internal/util:801:15\n' +
' at parseTypeScript (node:internal/modules/typescript:58:17)\n' +
' at processTypeScriptCode (node:internal/modules/typescript:146:42)\n' +
' at stripTypeScriptModuleTypes (node:internal/modules/typescript:209:22)\n' +
' at evalTypeScript (node:internal/process/execution:263:21)\n' +
' at node:internal/main/eval_string:71:3 {\n' +
" code: 'ERR_NO_TYPESCRIPT'\n" +
'}\n' +
'\n' +
'Node.js v26.0.0-pre\n'

at TestContext.<anonymous> (file:///Users/y-okt/ghq/github.com/y-okt/node/test/es-module/test-esm-detect-ambiguous.mjs:272:14) at async Test.run (node:internal/test_runner/test:1102:7) at async Suite.processPendingSubtests (node:internal/test_runner/test:777:7) {

generatedMessage: true,
code: 'ERR_ASSERTION',
actual: 'node:internal/process/execution:279\n' +
' throw tsError;\n' +
' ^\n' +
'\n' +
'Error [ERR_NO_TYPESCRIPT]: Node.js is not compiled with TypeScript support\n' +
' at assertTypeScript (node:internal/util:244:11)\n' +
' at node:internal/modules/typescript:46:3\n' +
' at node:internal/util:801:15\n' +
' at parseTypeScript (node:internal/modules/typescript:58:17)\n' +
' at processTypeScriptCode (node:internal/modules/typescript:146:42)\n' +
'...',
expected: /SyntaxError: await is only valid in async function/,
operator: 'match',
diff: 'simple'
}

but in this test we are not manually setting --strip-types to true, and since now is set to default false it should not fail.

I figured out why: we have to set bool strip_types = false; otherwise even if HAVE_AMARO is false it will also be true

diff --git a/src/node_options.h b/src/node_options.h index dd782b460ae..57a828de614 100644 --- a/src/node_options.h +++ b/src/node_options.h @@ -259,7 +259,7 @@ class EnvironmentOptions : public Options { std::vector<std::string> preload_esm_modules; - bool strip_types = true; + bool strip_types = false; bool experimental_transform_types = false; std::vector<std::string> user_argv;

Wouldn't false disable it for everyone?

diff --git a/src/node_options.h b/src/node_options.h index dd782b460ae..57a828de614 100644 --- a/src/node_options.h +++ b/src/node_options.h @@ -259,7 +259,7 @@ class EnvironmentOptions : public Options { std::vector<std::string> preload_esm_modules; - bool strip_types = true; + bool strip_types = HAVE_AMARO; bool experimental_transform_types = false; std::vector<std::string> user_argv;

No because the default: true in AddOption overrides it.
But also that works

aduh95 · 2025-11-23T17:05:50Z

test/parallel/test-worker-eval-typescript.js

+test('Worker eval module typescript without input-type',
+     { skip: !process.config.variables.node_use_amaro },
+     async () => {


You could make the diff more readable by using a variable

Suggested change

test('Worker eval module typescript without input-type',

{ skip: !process.config.variables.node_use_amaro },

async () => {

const onlyWithAmaro = { skip: !process.config.variables.node_use_amaro };

test('Worker eval module typescript without input-type', onlyWithAmaro, async () => {

marco-ippolito · 2025-11-23T17:19:08Z

Once we fix #60815 (comment)
it will be no longer necessary to skip these test files:

test/es-module/test-esm-detect-ambiguous.mjs
test/es-module/test-esm-tla-syntax-errors-not-recognized-as-tla-error.mjs
test/parallel/test-worker-cli-options.js
test/parallel/test-worker-load-file-with-extension-other-than-js.js
test/parallel/test-worker-syntax-error.js

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. config Issues or PRs related to the config subsystem needs-ci PRs that need a full CI run. labels Nov 22, 2025

y-okt mentioned this pull request Nov 22, 2025

--experimental-strip-types default value should depend on --without-amaro configure flag #60640

Open

aduh95 reviewed Nov 22, 2025

View reviewed changes

src/node_options.cc Outdated Show resolved Hide resolved

anonrig reviewed Nov 23, 2025

View reviewed changes

test/es-module/test-esm-import-meta-main-eval.mjs Outdated Show resolved Hide resolved

test/es-module/test-esm-tla-syntax-errors-not-recognized-as-tla-error.mjs Outdated Show resolved Hide resolved

piyush-pk reviewed Nov 23, 2025

View reviewed changes

src/node_options.cc Outdated Show resolved Hide resolved

y-okt added 4 commits November 23, 2025 23:25

test: skip failing tests when compiled without amaro

52acfc9

When compiled without amaro, this conflicts with --experimental-strip-types defaulting to true. To avoid that, we need to skip relevant failing tests. Fixes: nodejs#60640

src: make --strip-types option false when compiled without amaro

dced3f3

stripe-types option's default value is true, but we need to ensure that when compiled without amaro, the default vaue is false. Fixes: nodejs#60640

test: add workaround for the node-options-doc test to make the pipeli…

3dc852d

…ne pass fixed the test failure due to its changeable default value. Fixes: nodejs#60640

y-okt force-pushed the strip-types-default-amaro branch from a450ee5 to 3dc852d Compare November 23, 2025 14:32

y-okt commented Nov 23, 2025

View reviewed changes

y-okt requested review from aduh95 and anonrig and removed request for anonrig November 23, 2025 14:37

marco-ippolito reviewed Nov 23, 2025

View reviewed changes

y-okt requested a review from marco-ippolito November 23, 2025 15:52

aduh95 reviewed Nov 23, 2025

View reviewed changes

Uh oh!

test: skip failing tests and set the default value of strip-types to false when compiled without amaro #60815

Are you sure you want to change the base?

test: skip failing tests and set the default value of strip-types to false when compiled without amaro #60815

Conversation

y-okt commented Nov 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Background

Uh oh!

nodejs-github-bot commented Nov 22, 2025

Uh oh!

marco-ippolito commented Nov 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Nov 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

kapouer commented Nov 22, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

y-okt commented Nov 23, 2025

Uh oh!

y-okt Nov 23, 2025

Choose a reason for hiding this comment

Uh oh!

marco-ippolito Nov 23, 2025

Choose a reason for hiding this comment

Uh oh!

marco-ippolito Nov 23, 2025

Choose a reason for hiding this comment

Uh oh!

y-okt Nov 23, 2025

Choose a reason for hiding this comment

Uh oh!

marco-ippolito Nov 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marco-ippolito Nov 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aduh95 Nov 23, 2025

Choose a reason for hiding this comment

Uh oh!

marco-ippolito Nov 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aduh95 Nov 23, 2025

Choose a reason for hiding this comment

Uh oh!

marco-ippolito commented Nov 23, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

y-okt commented Nov 22, 2025 •

edited

Loading

marco-ippolito commented Nov 22, 2025 •

edited

Loading

codecov bot commented Nov 22, 2025 •

edited

Loading

marco-ippolito Nov 23, 2025 •

edited

Loading

marco-ippolito Nov 23, 2025 •

edited

Loading

marco-ippolito Nov 23, 2025 •

edited

Loading