test_runner: adds built in lcov test reporter by philnash · Pull Request #50018 · nodejs/node · GitHub

Review requested:

@nodejs/test_runner

@philnash could you please rebase on top of main? AFAIK merge commits break the CI

@atlowChemi Ah, sure. The GitHub interface just gave me that option. Will rebase and respond to your other comments too.

Thanks for the contribution 🎉

I don't know what's wrong with that one failed job. Anyone understand and can give me a pointer for how to fix?

This can be written more cleanly with modern stream machinery but I'm 100% OK with landing and then refactoring.

I've rerun the failed GH CI job it fail on an unrelated cluster test test-cluster-primary-kill.js I'll also trigger Jenkins CI

CI: https://ci.nodejs.org/job/node-test-pull-request/54500/

The test runner failures test-runner-output seem related:

18:57:19         # Subtest: test-runner/output/lcov_reporter.js
18:57:19         not ok 20 - test-runner/output/lcov_reporter.js
18:57:19           ---
18:57:19           duration_ms: 736.44068
18:57:19           location: 'file:///home/iojs/build/workspace/node-test-commit-linux-containered/test/parallel/test-runner-output.mjs:136:5'
18:57:19           failureType: 'testCodeFailure'
18:57:19           error: |-
18:57:19             Expected values to be strictly equal:
18:57:19             + actual - expected ... Lines skipped
18:57:19             
18:57:19             + '\n\n'
18:57:19             ...
18:57:19             - 'TN:\n' +
18:57:19             -   'SF:test/fixtures/test-runner/output/output.js\n' +
18:57:19             -   'FN:8,anonymous_0\n' +
18:57:19             -   'FN:12,anonymous_1\n' +
18:57:19             -   'FN:16,anonymous_2\n' +
18:57:19             -   'FN:21,anonymous_3\n' +
18:57:19             -   'FN:26,anonymous_4\n' +
18:57:19             -   'FN:30,anonymous_5\n' +
18:57:19             -   'FN:34,anonymous_6\n' +
18:57:19             -   'FN:38,anonymous_7\n' +
18:57:19             -   'FN:42,anonymous_8\n' +
18:57:19             -   'FN:46,anonymous_9\n' +
18:57:19             -   'FN:50,anonymous_10\n' +
18:57:19             -   'FN:54,anonymous_11\n' +
18:57:19             -   'FN:59,anonymous_12\n' +
18:57:19             -   'FN:64,anonymous_13\n' +
18:57:19             -   'FN:68,anonymous_14\n' +
18:57:19             -   'FN:72,anonymous_15\n' +
18:57:19             -   'FN:76,anonymous_16\n' +
18:57:19             -   'FN:80,anonymous_17\n' +
18:57:19             -   'FN:81,anonymous_18\n' +
18:57:19             -   'FN:86,anonymous_19\n' +
18:57:19             -   'FN:87,anonymous_20\n' +
18:57:19             -   'FN:92,anonymous_21\n' +
18:57:19             -   'FN:93,anonymous_22\n' +
18:57:19             -   'FN:94,anonymous_23\n' +
18:57:19             -   'FN:100,anonymous_24\n' +
18:57:19             -   'FN:101,anonymous_25\n' +
18:57:19             -   'FN:107,anonymous_26\n' +
18:57:19             -   'FN:111,anonymous_27\n' +
18:57:19             -   'FN:112,anonymous_28\n' +
18:57:19             -   'FN:113,anonymous_29\n' +
18:57:19             -   'FN:114,anonymous_30\n' +
18:57:19             -   'FN:122,anonymous_31\n' +
18:57:19             -   'FN:123,anonymous_32\n' +
18:57:19             -   'FN:130,anonymous_33\n' +
18:57:19             -   'FN:131,anonymous_34\n' +
18:57:19             -   'FN:132,anonymous_35\n' +
18:57:19             -   'FN:140,anonymous_36\n' +
18:57:19             -   'FN:141,anonymous_37\n' +
18:57:19             -   'FN:142,anonymous_38\n' +
18:57:19             -   'FN:150,anonymous_39\n' +
18:57:19             -   'FN:151,anonymous_40\n' +
18:57:19             -   'FN:159,anonymous_41\n' +
18:57:19             -   'FN:160,anonymous_42\n' +
18:57:19             -   'FN:161,anonymous_43\n' +
18:57:19             -   'FN:166,anonymous_44\n' +
18:57:19             -   'FN:167,anonymous_45\n' +
18:57:19             -   'FN:171,anonymous_46\n' +
18:57:19             -   'FN:172,anonymous_47\n' +
18:57:19             ...
18:57:19           code: 'ERR_ASSERTION'
18:57:19           name: 'AssertionError'

Note this is flaky - it failed on one machine but not others

Two nits - prefer the primordial (ArrayPrototypeSort) and you can do [...file.lines].sort(/**/) as file.lines.toSorted(/**/)

I originally had this as toSorted but I got in my head about using newer JS features for some reason. Though on that note, should something like toSorted be a primordial too?

I've changed this to toSorted. I couldn't find how to define a new primordial. I'd be happy to add one if someone can point me in the right direction.

@philnash primordials should exist for any function that exists on the prototype...

node/lib/internal/per_context/primordials.js

Lines 184 to 230 in 0171933

    
           // Create copies of intrinsic objects 
        
           [ 
        
             'AggregateError', 
        
             'Array', 
        
             'ArrayBuffer', 
        
             'BigInt', 
        
             'BigInt64Array', 
        
             'BigUint64Array', 
        
             'Boolean', 
        
             'DataView', 
        
             'Date', 
        
             'Error', 
        
             'EvalError', 
        
             'FinalizationRegistry', 
        
             'Float32Array', 
        
             'Float64Array', 
        
             'Function', 
        
             'Int16Array', 
        
             'Int32Array', 
        
             'Int8Array', 
        
             'Map', 
        
             'Number', 
        
             'Object', 
        
             'RangeError', 
        
             'ReferenceError', 
        
             'RegExp', 
        
             'Set', 
        
             'String', 
        
             'Symbol', 
        
             'SyntaxError', 
        
             'TypeError', 
        
             'URIError', 
        
             'Uint16Array', 
        
             'Uint32Array', 
        
             'Uint8Array', 
        
             'Uint8ClampedArray', 
        
             'WeakMap', 
        
             'WeakRef', 
        
             'WeakSet', 
        
           ].forEach((name) => { 
        
             // eslint-disable-next-line no-restricted-globals 
        
             const original = globalThis[name]; 
        
             primordials[name] = original; 
        
             copyPropsRenamed(original, primordials, name); 
        
             copyPrototype(original.prototype, primordials, `${name}Prototype`); 
        
           });

Note this is flaky - it failed on one machine but not others

I think this test might need to be disabled on machines without inspector. see

node/test/parallel/test-runner-coverage.js

Lines 9 to 11 in 0171933

    
           const skipIfNoInspector = { 
        
             skip: !process.features.inspector ? 'inspector disabled' : false 
        
           };

I think the order should be:

Suggested change

      
                it(name, fn, options);
          
                it(name, options, fn);

node/doc/api/test.md

Line 1075 in 0171933

## `it([name][, options][, fn])`

node/lib/internal/test_runner/test.js

Lines 423 to 425 in 0171933

    
           } else if (typeof options === 'function') { 
        
             fn = options; 
        
           }

Oh, you are correct. 🤦‍♂️ So the actual options here were just getting ignored and the tests still passed.

CI: https://ci.nodejs.org/job/node-test-pull-request/54534/

Ah, those are fails because there was no inspector. The test was skipped, but the test also included a spy saying it must be called, so it failed for that reason. Readjusting the test skipping now.

Landed in c60c11a

Fixes #49626 PR-URL: #50018 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

Notable changes: doc: * add MrJithil to collaborators (Jithil P Ponnan) #50666 * add Ethan-Arrowood as a collaborator (Ethan Arrowood) #50393 esm: * (SEMVER-MINOR) add import.meta.dirname and import.meta.filename (James Sumners) #48740 fs: * add stacktrace to fs/promises (翠 / green) #49849 lib: * (SEMVER-MINOR) add navigator.platform (Aras Abbasi) #50385 stream: * (SEMVER-MINOR) add support for `deflate-raw` format to webstreams compression (Damian Krzeminski) #50097 * use Array for Readable buffer (Robert Nagy) #50341 * optimize creation (Robert Nagy) #50337 test_runner: * adds built in lcov reporter (Phil Nash) #50018 * (SEMVER-MINOR) add Date to the supported mock APIs (Lucas Santos) #48638 test_runner, cli: * (SEMVER-MINOR) add --test-timeout flag (Shubham Pandey) #50443 PR-URL: #50681

This is semver-minor. Collaborators, please think about adding the label in future pull requests. It will make the work of releasers easier.

Notable changes: doc: * add MrJithil to collaborators (Jithil P Ponnan) #50666 * add Ethan-Arrowood as a collaborator (Ethan Arrowood) #50393 esm: * (SEMVER-MINOR) add import.meta.dirname and import.meta.filename (James Sumners) #48740 fs: * add stacktrace to fs/promises (翠 / green) #49849 lib: * (SEMVER-MINOR) add `--no-experimental-global-navigator` CLI flag (Antoine du Hamel) #50562 * (SEMVER-MINOR) add navigator.language & navigator.languages (Aras Abbasi) #50303 * (SEMVER-MINOR) add navigator.platform (Aras Abbasi) #50385 stream: * (SEMVER-MINOR) add support for `deflate-raw` format to webstreams compression (Damian Krzeminski) #50097 * use Array for Readable buffer (Robert Nagy) #50341 * optimize creation (Robert Nagy) #50337 test_runner: * (SEMVER-MINOR) adds built in lcov reporter (Phil Nash) #50018 * (SEMVER-MINOR) add Date to the supported mock APIs (Lucas Santos) #48638 test_runner, cli: * (SEMVER-MINOR) add --test-timeout flag (Shubham Pandey) #50443 PR-URL: #50681

Fixes #49626 PR-URL: #50018 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

Notable changes: crypto: * update root certificates to NSS 3.95 (Node.js GitHub Bot) #50805 doc: * add MrJithil to collaborators (Jithil P Ponnan) #50666 * add Ethan-Arrowood as a collaborator (Ethan Arrowood) #50393 esm: * (SEMVER-MINOR) add import.meta.dirname and import.meta.filename (James Sumners) #48740 fs: * add c++ fast path for writeFileSync utf8 (CanadaHonk) #49884 module: * (SEMVER-MINOR) remove useCustomLoadersIfPresent flag (Chengzhong Wu) #48655 src: * (SEMVER-MINOR) add `--disable-warning` option (Ethan Arrowood) #50661 * (SEMVER-MINOR) create per isolate proxy env template (Chengzhong Wu) #48655 * (SEMVER-MINOR) make process binding data weak (Chengzhong Wu) #48655 stream: * use Array for Readable buffer (Robert Nagy) #50341 * optimize creation (Robert Nagy) #50337 test_runner: * (SEMVER-MINOR) adds built in lcov reporter (Phil Nash) #50018 * (SEMVER-MINOR) add Date to the supported mock APIs (Lucas Santos) #48638 test_runner, cli: * (SEMVER-MINOR) add --test-timeout flag (Shubham Pandey) #50443 PR-URL: TODO

Notable changes: crypto: * update root certificates to NSS 3.95 (Node.js GitHub Bot) #50805 doc: * add MrJithil to collaborators (Jithil P Ponnan) #50666 * add Ethan-Arrowood as a collaborator (Ethan Arrowood) #50393 esm: * (SEMVER-MINOR) add import.meta.dirname and import.meta.filename (James Sumners) #48740 fs: * add c++ fast path for writeFileSync utf8 (CanadaHonk) #49884 module: * (SEMVER-MINOR) remove useCustomLoadersIfPresent flag (Chengzhong Wu) #48655 src: * (SEMVER-MINOR) add `--disable-warning` option (Ethan Arrowood) #50661 * (SEMVER-MINOR) create per isolate proxy env template (Chengzhong Wu) #48655 * (SEMVER-MINOR) make process binding data weak (Chengzhong Wu) #48655 stream: * use Array for Readable buffer (Robert Nagy) #50341 * optimize creation (Robert Nagy) #50337 test_runner: * (SEMVER-MINOR) adds built in lcov reporter (Phil Nash) #50018 * (SEMVER-MINOR) add Date to the supported mock APIs (Lucas Santos) #48638 test_runner, cli: * (SEMVER-MINOR) add --test-timeout flag (Shubham Pandey) #50443 PR-URL: #51124

Notable changes: crypto: * update root certificates to NSS 3.95 (Node.js GitHub Bot) #50805 doc: * add MrJithil to collaborators (Jithil P Ponnan) #50666 * add Ethan-Arrowood as a collaborator (Ethan Arrowood) #50393 esm: * (SEMVER-MINOR) add import.meta.dirname and import.meta.filename (James Sumners) #48740 fs: * add c++ fast path for writeFileSync utf8 (CanadaHonk) #49884 module: * (SEMVER-MINOR) remove useCustomLoadersIfPresent flag (Chengzhong Wu) #48655 * (SEMVER-MINOR) bootstrap module loaders in shadow realm (Chengzhong Wu) #48655 src: * (SEMVER-MINOR) add `--disable-warning` option (Ethan Arrowood) #50661 * (SEMVER-MINOR) create per isolate proxy env template (Chengzhong Wu) #48655 * (SEMVER-MINOR) make process binding data weak (Chengzhong Wu) #48655 stream: * use Array for Readable buffer (Robert Nagy) #50341 * optimize creation (Robert Nagy) #50337 test_runner: * (SEMVER-MINOR) adds built in lcov reporter (Phil Nash) #50018 * (SEMVER-MINOR) add Date to the supported mock APIs (Lucas Santos) #48638 test_runner, cli: * (SEMVER-MINOR) add --test-timeout flag (Shubham Pandey) #50443 PR-URL: #51124

Notable changes: crypto: * update root certificates to NSS 3.95 (Node.js GitHub Bot) nodejs#50805 doc: * add MrJithil to collaborators (Jithil P Ponnan) nodejs#50666 * add Ethan-Arrowood as a collaborator (Ethan Arrowood) nodejs#50393 esm: * (SEMVER-MINOR) add import.meta.dirname and import.meta.filename (James Sumners) nodejs#48740 fs: * add c++ fast path for writeFileSync utf8 (CanadaHonk) nodejs#49884 module: * (SEMVER-MINOR) remove useCustomLoadersIfPresent flag (Chengzhong Wu) nodejs#48655 * (SEMVER-MINOR) bootstrap module loaders in shadow realm (Chengzhong Wu) nodejs#48655 src: * (SEMVER-MINOR) add `--disable-warning` option (Ethan Arrowood) nodejs#50661 * (SEMVER-MINOR) create per isolate proxy env template (Chengzhong Wu) nodejs#48655 * (SEMVER-MINOR) make process binding data weak (Chengzhong Wu) nodejs#48655 stream: * use Array for Readable buffer (Robert Nagy) nodejs#50341 * optimize creation (Robert Nagy) nodejs#50337 test_runner: * (SEMVER-MINOR) adds built in lcov reporter (Phil Nash) nodejs#50018 * (SEMVER-MINOR) add Date to the supported mock APIs (Lucas Santos) nodejs#48638 test_runner, cli: * (SEMVER-MINOR) add --test-timeout flag (Shubham Pandey) nodejs#50443 PR-URL: nodejs#51124

nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. test_runner Issues and PRs related to the test runner subsystem. labels Oct 2, 2023

anonrig reviewed Oct 2, 2023

View reviewed changes

Comment thread lib/internal/test_runner/reporter/lcov.js Outdated

Comment thread test/fixtures/test-runner/output/lcov_reporter.js Outdated

atlowChemi reviewed Oct 3, 2023

View reviewed changes

Comment thread lib/test/reporters.js Outdated

Comment thread doc/api/test.md Outdated

Comment thread lib/internal/test_runner/reporter/lcov.js Outdated

philnash force-pushed the test-runner-lcov-reporter branch from 7fb19d3 to 44a2ee2 Compare October 3, 2023 09:32

MoLow reviewed Oct 3, 2023

View reviewed changes

Comment thread lib/internal/test_runner/reporter/lcov.js Outdated

MoLow approved these changes Oct 3, 2023

View reviewed changes

MoLow added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Oct 3, 2023

ljharb requested changes Oct 3, 2023

View reviewed changes

Comment thread lib/internal/test_runner/reporter/lcov.js Outdated

Comment thread lib/internal/test_runner/reporter/lcov.js Outdated

benjamingr reviewed Oct 3, 2023

View reviewed changes

benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 3, 2023

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 3, 2023

benjamingr reviewed Oct 3, 2023

View reviewed changes

Comment thread lib/internal/test_runner/reporter/lcov.js Outdated

atlowChemi reviewed Oct 4, 2023

View reviewed changes

atlowChemi added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 4, 2023

atlowChemi approved these changes Oct 4, 2023

View reviewed changes

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 4, 2023

nodejs-github-bot merged commit c60c11a into nodejs:main Oct 25, 2023

This was referenced Oct 26, 2023

CI Reliability 2023-10-26 nodejs/reliability#698

Open

CI Reliability 2023-10-27 nodejs/reliability#699

Open

CI Reliability 2023-10-28 nodejs/reliability#700

Open

CI Reliability 2023-10-29 nodejs/reliability#701

Open

targos pushed a commit that referenced this pull request Nov 11, 2023

test_runner: adds built in lcov reporter

c9b92bb

Fixes #49626 PR-URL: #50018 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

targos mentioned this pull request Nov 12, 2023

v21.2.0 release proposal #50681

Merged

targos added the semver-minor PRs that contain new features and should be released in the next minor version. label Nov 13, 2023

UlisesGascon pushed a commit that referenced this pull request Dec 11, 2023

test_runner: adds built in lcov reporter

e643786

Fixes #49626 PR-URL: #50018 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

UlisesGascon mentioned this pull request Dec 12, 2023

v20.11.0 proposal #51124

Merged

	// Create copies of intrinsic objects
	[
	'AggregateError',
	'Array',
	'ArrayBuffer',
	'BigInt',
	'BigInt64Array',
	'BigUint64Array',
	'Boolean',
	'DataView',
	'Date',
	'Error',
	'EvalError',
	'FinalizationRegistry',
	'Float32Array',
	'Float64Array',
	'Function',
	'Int16Array',
	'Int32Array',
	'Int8Array',
	'Map',
	'Number',
	'Object',
	'RangeError',
	'ReferenceError',
	'RegExp',
	'Set',
	'String',
	'Symbol',
	'SyntaxError',
	'TypeError',
	'URIError',
	'Uint16Array',
	'Uint32Array',
	'Uint8Array',
	'Uint8ClampedArray',
	'WeakMap',
	'WeakRef',
	'WeakSet',
	].forEach((name) => {
	// eslint-disable-next-line no-restricted-globals
	const original = globalThis[name];
	primordials[name] = original;
	copyPropsRenamed(original, primordials, name);
	copyPrototype(original.prototype, primordials, `${name}Prototype`);
	});

test_runner: adds built in lcov test reporter by philnash · Pull Request #50018 · nodejs/node · GitHub

Uh oh!

Uh oh!

test_runner: adds built in lcov test reporter - #50018

test_runner: adds built in lcov test reporter#50018nodejs-github-bot merged 14 commits intonodejs:mainnodejs/node:mainfrom philnash:test-runner-lcov-reporterphilnash/node:test-runner-lcov-reporterCopy head branch name to clipboard

Conversation

philnash commented Oct 2, 2023

Uh oh!

nodejs-github-bot commented Oct 2, 2023

Uh oh!

Uh oh!

Uh oh!

atlowChemi commented Oct 3, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

philnash commented Oct 3, 2023

Uh oh!

Uh oh!

MoLow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

philnash commented Oct 3, 2023

Uh oh!

benjamingr Oct 3, 2023

Choose a reason for hiding this comment

Uh oh!

benjamingr commented Oct 3, 2023

Uh oh!

nodejs-github-bot commented Oct 3, 2023

Uh oh!

benjamingr commented Oct 3, 2023

Uh oh!

benjamingr commented Oct 3, 2023

Uh oh!

benjamingr Oct 3, 2023

Choose a reason for hiding this comment

Uh oh!

philnash Oct 3, 2023

Choose a reason for hiding this comment

Uh oh!

philnash Oct 3, 2023

Choose a reason for hiding this comment

Uh oh!

atlowChemi Oct 4, 2023 • 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!

Uh oh!

atlowChemi commented Oct 3, 2023

Uh oh!

atlowChemi Oct 4, 2023 • 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!

philnash Oct 4, 2023

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Oct 4, 2023

Uh oh!

philnash commented Oct 4, 2023

Uh oh!

nodejs-github-bot commented Oct 25, 2023

Uh oh!

targos commented Nov 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

test_runner: adds built in lcov test reporter#50018
nodejs-github-bot merged 14 commits into
nodejs:mainfrom
philnash:test-runner-lcov-reporter

atlowChemi Oct 4, 2023 •
edited
Loading

atlowChemi Oct 4, 2023 •
edited
Loading