Skip to content

Conversation

@ericcrosson-bitgo
Copy link
Contributor

@ericcrosson-bitgo ericcrosson-bitgo commented May 28, 2023

This PR replaces ava with the Node.js native test runner1.

The refactor was straightforward in the express-wrapper and the typed-express-wrapper, but there are some notable changes in the openapi-generator.

The naive implementation ran each test in the test corpus serially, due to this nature of the native test runner's execution model2:

Each test file is executed as if it was a regular script. That is,
if the test file itself uses node:test to define tests, all of those
tests will be executed within a single application thread, regardless
of the value of the concurrency option of test().

Since the function under test is synchronous and compute-bound, and Node.js is single-threaded, all the tests were executing serially and took over 30s to complete on my macbook.

Consequently, this commit uses Node.js worker threads3 to spawn one worker per test-corpus file, limited to one worker per CPU core. In this configuration, all tests complete in 17s on my macbook.

Supersedes #465

Footnotes

  1. https://nodejs.org/api/test.html

  2. https://nodejs.org/api/test.html#test-runner-execution-model

  3. https://nodejs.org/api/worker_threads.html

This bumps the Node.js version from 18.12 to 18.16, which includes a fix
for a segfault when using c8 for code-coverage collection.
This commit replaces ava with the Node.js native test runner[^1].

The naive implementation ran each test in the test corpus serially, due
to this nature of the native test runner's execution model[^2]:

> Each test file is executed as if it was a regular script. That is,
> if the test file itself uses node:test to define tests, all of those
> tests will be executed within a single application thread, regardless
> of the value of the concurrency option of test().

Since the function under test is synchronous and compute-bound, and
Node.js is single-threaded, all the tests were executing serially and
took over 30s to complete on my macbook.

Consequently, this commit uses Node.js worker threads[^3] to spawn one
worker per test-corpus file, limited to one worker per CPU core. In this
configuration, all tests complete in 17s on my macbook.

[^1]: https://nodejs.org/api/test.html
[^2]: https://nodejs.org/api/test.html#test-runner-execution-model
[^3]: https://nodejs.org/api/worker_threads.html
@socket-security
Copy link

New dependency changes detected. Learn more about Socket for GitHub ↗︎


🚨 Potential security issues found in this pull request. To accept the risk, merge this PR and you will not be notified again.

Bot Commands

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of package-name@version specifiers. e.g. @SocketSecurity ignore foo@1.0.0 bar@* or ignore all packages with @SocketSecurity ignore-all

  • @SocketSecurity ignore esbuild@0.17.19
📜 Install scripts

Install scripts are run when the package is installed. The majority of malware in npm is hidden in install scripts.

Packages should not be running non-essential scripts during install and there are often solutions to problems people solve with install scripts that can be run at publish time instead.

Package Script field Source
esbuild@0.17.19 (added) postinstall package-lock.json, packages/express-wrapper/package.json via tsx@3.12.7, packages/openapi-generator/package.json via tsx@3.12.7, packages/typed-express-router/package.json via tsx@3.12.7
Pull request alert summary
Issue Status
Install scripts ⚠️ 1 issue
Native code ✅ 0 issues
Bin script shell injection ✅ 0 issues
Unresolved require ✅ 0 issues
Invalid package.json ✅ 0 issues
HTTP dependency ✅ 0 issues
Git dependency ✅ 0 issues
Potential typo squat ✅ 0 issues
Known Malware ✅ 0 issues
Telemetry ✅ 0 issues
Protestware/Troll package ✅ 0 issues

📊 Modified Dependency Overview:

➕ Added Package Capability Access +/- Transitive Count Publisher
tsx@3.12.7 eval, network, filesystem, shell, environment +81 hirokiosame

🚮 Removed packages: @ava/typescript@3.0.1, ava@5.2.0

@ericcrosson-bitgo ericcrosson-bitgo force-pushed the build-nix-upgrade-nixpkgs-flake-input branch from 9237601 to 40d3123 Compare May 28, 2023 00:40
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please note this diff, where we necessarily stop running unit tests on Node.js 14 and 16. I personally am all right with this change, but would love to hear what you think!

@ericcrosson-bitgo ericcrosson-bitgo marked this pull request as ready for review May 28, 2023 00:45
@ericcrosson-bitgo ericcrosson-bitgo requested a review from a team as a code owner May 28, 2023 00:45
Copy link
Contributor

@bitgopatmcl bitgopatmcl left a comment

Choose a reason for hiding this comment

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

This works :shipit:

@ericcrosson-bitgo ericcrosson-bitgo merged commit 266e2d6 into BitGo:master Jun 5, 2023
@github-actions
Copy link

github-actions bot commented Jun 8, 2023

🎉 This PR is included in version @api-ts/io-ts-http@2.4.0 🎉

The release is available on npm package (@latest dist-tag)

Your semantic-release bot 📦🚀

@github-actions
Copy link

github-actions bot commented Jun 8, 2023

🎉 This PR is included in version @api-ts/express-wrapper@1.0.18 🎉

The release is available on npm package (@latest dist-tag)

Your semantic-release bot 📦🚀

@github-actions
Copy link

github-actions bot commented Jun 8, 2023

🎉 This PR is included in version @api-ts/openapi-generator@1.0.9 🎉

The release is available on npm package (@latest dist-tag)

Your semantic-release bot 📦🚀

@github-actions
Copy link

github-actions bot commented Jun 8, 2023

🎉 This PR is included in version @api-ts/superagent-wrapper@1.1.11 🎉

The release is available on npm package (@latest dist-tag)

Your semantic-release bot 📦🚀

@github-actions
Copy link

github-actions bot commented Jun 8, 2023

🎉 This PR is included in version @api-ts/typed-express-router@1.1.0 🎉

The release is available on npm package (@latest dist-tag)

Your semantic-release bot 📦🚀

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants