Skip to content

test: enable 2 already-passing Node compat tests#33707

Closed
nathanwhitbot wants to merge 1 commit intodenoland:mainfrom
nathanwhitbot:fix/node-compat-iter73
Closed

test: enable 2 already-passing Node compat tests#33707
nathanwhitbot wants to merge 1 commit intodenoland:mainfrom
nathanwhitbot:fix/node-compat-iter73

Conversation

@nathanwhitbot
Copy link
Copy Markdown
Contributor

Summary

Enables 2 Node.js compat tests that already pass on main without any code changes. No production code touched — just tests/node_compat/config.jsonc entries.

  • parallel/test-startup-empty-regexp-statics.mjs
  • parallel/test-tls-handshake-exception.js

The .mjs regexp-statics test (unlike its .js sibling) skips the failing deno -p spawn assertion and only checks the static RegExp.$_ / RegExp.$1 / etc. defaults — which Deno gets right.

Test plan

  • Each test verified passing 5/5 (mjs) and 3/3 (tls) times via the node_compat test runner
  • Full cargo test --test node_compat run on Linux confirms no regressions (only pre-existing flaky failures)

@nathanwhitbot nathanwhitbot force-pushed the fix/node-compat-iter73 branch from fe26f0d to ebe93ba Compare April 29, 2026 17:02
@nathanwhitbot
Copy link
Copy Markdown
Contributor Author

Force-pushed ebe93ba1d — branch was stacked on #33653 as a base. Reset to upstream/main and cherry-picked just the 2-test commit. Diff is now 1 file, +2 -0.

Copy link
Copy Markdown
Contributor Author

@nathanwhitbot nathanwhitbot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Self-review of `ebe93ba1d` (post-cleanup):

Pure config-only test enrollment, no production code. Both entries alphabetically positioned correctly:

Test Prev Next
`test-startup-empty-regexp-statics.mjs` `test-stack-size-limit.js` `test-stdin-child-proc.js`
`test-tls-handshake-exception.js` `test-tls-handshake-error.js` `test-tls-honorcipherorder.js`

Both enabled with empty config (`{}`). Body's note about `.mjs` vs `.js` regexp-statics is accurate — the `.mjs` test only validates static `RegExp.$_`/`$1`/etc. defaults without spawning a `deno -p` subprocess that the `.js` version uses. Holding to COMMENT until CI confirms.

Copy link
Copy Markdown
Contributor Author

@nathanwhitbot nathanwhitbot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Single-commit, 2-line config-only enrollment. No scope-creep. LGTM — TLS handshake test can be timing-sensitive on slower runners; if either test comes back red on Windows or macOS drop rather than gate.

@nathanwhitbot
Copy link
Copy Markdown
Contributor Author

Both tests in this PR fail on windows-x86_64:

  • test-startup-empty-regexp-statics.mjs — assertion failure (shard 1/3)
  • test-tls-handshake-exception.js — timeout 10s (shard 3/3)

Drop both per @bartlomieju's convention.

(Shard 2/3 failure on test-dns-resolver-max-timeout.js is unrelated timing flake, not in this PR's scope.)

@nathanwhitbot
Copy link
Copy Markdown
Contributor Author

Closing — both enabled tests fail on windows-x86_64 (test-startup-empty-regexp-statics.mjs assertion, test-tls-handshake-exception.js timeout). Per @bartlomieju's drop-rather-than-gate convention from #33593, dropping both leaves nothing to enroll. Will revisit individually if either turns out to be Deno-fixable rather than a genuine Windows incompatibility.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants