chore: Switch to using Node built-in test runner #2387

jsumners-nr · 2024-07-22T16:33:52Z

This PR resolves #2379. It must target the next branch because Node.js 16 does not include the test runner.

jsumners-nr · 2024-07-22T16:34:45Z

package.json

@@ -161,22 +161,22 @@
    "bench": "node ./bin/run-bench.js",
    "docker-env": "./bin/docker-env-vars.sh",
    "docs": "rm -rf ./out && jsdoc -c ./jsdoc-conf.jsonc --private -r .",
-    "integration": "npm run prepare-test && npm run sub-install && time c8 -o ./coverage/integration tap --test-regex='(\\/|^test\\/integration\\/.*\\.tap\\.js)$' --timeout=600 --no-coverage --reporter classic",
-    "integration:esm": "time c8 -o ./coverage/integration-esm tap --node-arg='--loader=./esm-loader.mjs' --test-regex='(test\\/integration\\/.*\\.tap\\.mjs)$' --timeout=600 --no-coverage --reporter classic",
+    "integration": "npm run prepare-test && npm run sub-install && time c8 -o ./coverage/integration node --test test/integration/**/*.tap.js",


I don't like that we have to rely on shell expansion of the globs in all of these scripts. But we don't run CI on Windows, and in-process expansion of the globs isn't supported until Node 22. So it is what it is.

bizob2828

I have notice the default reporter spec could pose problems in CI. It does not appear there is an equivalent to tap's classic reporter. If we change to dot reporter it will reduce I/O and seems to speed up the test runs as well. This may have to be a follow up because --test-reporter doesn't exist until 18

I did suggest some changes as I noticed the globs didn't account for tests in the direct child of a given test type. However, it also appears my proposed changes to include all tests by chaining globs segfaults in node 16, and does not work in 18 and 20.

I did compare the coverage is still the same, as expected because we're still relying on v8 coverage and c8 to report it.

package.json

jsumners-nr · 2024-07-23T11:52:37Z

I have notice the default reporter spec could pose problems in CI.

What problems might those be?

If we change to dot reporter it will reduce I/O and seems to speed up the test runs as well.

I don't particularly like the dot reporter because it is very cryptic. But putting the speed theory to the test:

# Unit test run without specifying a reporter:
ℹ tests 176
ℹ suites 0
ℹ pass 176
ℹ fail 0
ℹ cancelled 0
ℹ skipped 0
ℹ todo 0
ℹ duration_ms 31175.115292

real    0m34.861s
user    1m25.072s
sys     0m31.439s

# With the dot reporter:
................................................................................................................................................................................

real    0m29.611s
user    1m25.536s
sys     0m32.046s

I've done the test multiple times and either reporter is roughly the same amount of time.

One key difference is in error reporting. This is what the dot reporter does if there is a test failure:

❯ npm run unit

> newrelic@11.23.1 unit
> rm -f newrelic_agent.log && time c8 -o ./coverage/unit node --test-reporter dot --test test/unit/*.test.js test/unit/**/*.test.js

.......................................X........................................................................................................................................

real    0m28.944s
user    1m25.835s
sys     0m31.681s

I think that is really useless. I don't enjoy having to search all of the output of the standard reporter, but at least we can see which test(s) failed.

jsumners-nr · 2024-07-23T12:20:56Z

I think that is really useless. I don't enjoy having to search all of the output of the standard reporter, but at least we can see which test(s) failed.

🤔 we could report to stdout with the dot reporter but report to something like unit.txt via stderr with the spec reporter.

We could also write a simple reporter that outputs test failures when they occur, and nothing on success. In other words, mimic our versioned tests output. Example:

❯ npm run unit

> newrelic@11.23.1 unit
> rm -f newrelic_agent.log && time c8 -o ./coverage/unit node --test-reporter ./reporter.js --test test/unit/*.test.js test/unit/**/*.test.js

failed: /Users/jsumners/Projects/team-repos/node-newrelic/test/unit/attributes.test.js
real    0m26.942s
user    1m23.630s
sys     0m31.549s

I think we could write a much better reporter that shows offending line numbers and such, but the runner in v18 seems to omit a lot of that in my test case (just reporting line = 1; column = 1 for the induced failure when it should at least be line 35). But I think that report is enough to give us more to go on than the dot reporter and doesn't spam stdout with difficult to sift through text.

bizob2828

the update to globs still isn't running all the tests. i found in node 22 if you wrap each glob in single quotes it works. but that fails in 18 and 20

test/lib/test-reporter.js

bizob2828

overall looks great. i do have some failing tests that i suspect are because of time outs though

test/unit/attributes.test.js

bizob2828 · 2024-07-25T18:25:21Z

package.json

    "ssl": "./bin/ssl.sh",
    "sub-install": "node test/bin/install_sub_deps",
    "test": "npm run integration && npm run unit",
    "third-party-updates": "oss third-party manifest --includeOptDeps && oss third-party notices --includeOptDeps && git add THIRD_PARTY_NOTICES.md third_party_manifest.json",
-    "unit": "rm -f newrelic_agent.log && time c8 -o ./coverage/unit tap --test-regex='(\\/|^test\\/unit\\/.*\\.test\\.js)$' --timeout=180 --no-coverage --reporter classic",
-    "unit:scripts": "time c8 -o ./coverage/scripts-unit tap --test-regex='(\\/|^bin\\/test\\/.*\\.test\\.js)$' --timeout=180 --no-coverage --reporter classic",
+    "unit": "rm -f newrelic_agent.log && time c8 -o ./coverage/unit borp --reporter ./test/lib/test-reporter.mjs 'test/unit/**/*.test.js'",


i think a timeout has to be set. the facts.test.js fails

bizob2828 · 2024-07-25T18:27:39Z

package.json

    "prepare-test": "npm run ssl && npm run docker-env",
    "lint": "eslint ./*.{js,mjs} lib test bin examples",
    "lint:fix": "eslint --fix, ./*.{js,mjs} lib test bin examples",
    "public-docs": "jsdoc -c ./jsdoc-conf.jsonc && cp examples/shim/*.png out/",
    "publish-docs": "./bin/publish-docs.sh",
    "services": "DOCKER_PLATFORM=linux/$(uname -m) docker compose up -d --wait",
    "services:stop": "docker compose down",
-    "smoke": "npm run ssl && time tap test/smoke/**/**/*.tap.js --timeout=180 --no-coverage",
+    "smoke": "npm run ssl && time borp --reporter ./test/lib/test-reporter.mjs 'test/smoke/**/*.tap.js'",


i think we need to restore the timeout. i have 1 test that fails

bizob2828 · 2024-07-25T18:28:59Z

test/lib/test-reporter.mjs

+const OUTPUT_MODE = process.env.OUTPUT_MODE?.toLowerCase() ?? 'simple'
+const isSilent = OUTPUT_MODE === 'quiet' || OUTPUT_MODE === 'silent'
+
+async function* reporter(source) {


very nice. we can always add to this.

jsumners-nr commented Jul 22, 2024

View reviewed changes

jsumners-nr marked this pull request as ready for review July 22, 2024 16:34

bizob2828 requested changes Jul 22, 2024

View reviewed changes

package.json Outdated Show resolved Hide resolved

package.json Outdated Show resolved Hide resolved

jsumners-nr force-pushed the issue-2379 branch from 702af7a to 6bd0072 Compare July 23, 2024 14:24

jsumners-nr requested a review from bizob2828 July 23, 2024 14:24

jsumners-nr force-pushed the issue-2379 branch from 6bd0072 to 9090e1a Compare July 23, 2024 14:47

jsumners-nr added the dev:tests Indicates only changes to tests label Jul 24, 2024

bizob2828 requested changes Jul 24, 2024

View reviewed changes

test/lib/test-reporter.js Outdated Show resolved Hide resolved

jsumners-nr added 4 commits July 25, 2024 10:17

chore: Switch to using Node built-in test runner

c7d2d69

address feedback

2eadfe3

make reporter honor OUTPUT_MODE

ea076a7

use borp to solve expansion woes

19ffc51

jsumners-nr force-pushed the issue-2379 branch from 08668a1 to 19ffc51 Compare July 25, 2024 14:17

integration tests insane timeout

a2efcec

jsumners-nr requested a review from bizob2828 July 25, 2024 14:53

add summary report to reporter

9e27541

jsumners-nr force-pushed the issue-2379 branch from cd42509 to 9e27541 Compare July 25, 2024 18:19

bizob2828 requested changes Jul 25, 2024

View reviewed changes

add timeouts to tests

6880d3e

jsumners-nr requested a review from bizob2828 July 25, 2024 18:58

bizob2828 approved these changes Jul 25, 2024

View reviewed changes

jsumners-nr merged commit b9f64b7 into newrelic:next Jul 25, 2024
23 checks passed

jsumners-nr deleted the issue-2379 branch July 25, 2024 19:19

github-actions bot mentioned this pull request Jul 31, 2024

chore: release v12.0.0 #2418

Merged

bizob2828 mentioned this pull request Jul 31, 2024

chore: Merge next branch #2412

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Switch to using Node built-in test runner #2387

chore: Switch to using Node built-in test runner #2387

jsumners-nr commented Jul 22, 2024

jsumners-nr Jul 22, 2024

bizob2828 left a comment

jsumners-nr commented Jul 23, 2024 •

edited

Loading

jsumners-nr commented Jul 23, 2024

bizob2828 left a comment

bizob2828 left a comment

bizob2828 Jul 25, 2024

bizob2828 Jul 25, 2024

bizob2828 Jul 25, 2024

chore: Switch to using Node built-in test runner #2387

chore: Switch to using Node built-in test runner #2387

Conversation

jsumners-nr commented Jul 22, 2024

jsumners-nr Jul 22, 2024

Choose a reason for hiding this comment

bizob2828 left a comment

Choose a reason for hiding this comment

jsumners-nr commented Jul 23, 2024 • edited Loading

jsumners-nr commented Jul 23, 2024

bizob2828 left a comment

Choose a reason for hiding this comment

bizob2828 left a comment

Choose a reason for hiding this comment

bizob2828 Jul 25, 2024

Choose a reason for hiding this comment

bizob2828 Jul 25, 2024

Choose a reason for hiding this comment

bizob2828 Jul 25, 2024

Choose a reason for hiding this comment

jsumners-nr commented Jul 23, 2024 •

edited

Loading