test_runner: adds built in lcov test reporter #50018

philnash · 2023-10-02T23:46:17Z

Fixes #49626

nodejs-github-bot · 2023-10-02T23:46:21Z

Review requested:

@nodejs/test_runner

lib/internal/test_runner/reporter/lcov.js

test/fixtures/test-runner/output/lcov_reporter.js

atlowChemi · 2023-10-03T07:23:48Z

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

lib/test/reporters.js

doc/api/test.md

lib/internal/test_runner/reporter/lcov.js

philnash · 2023-10-03T07:43:07Z

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

lib/internal/test_runner/reporter/lcov.js

MoLow

Thanks for the contribution 🎉

lib/internal/test_runner/reporter/lcov.js

philnash · 2023-10-03T13:00:31Z

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

benjamingr · 2023-10-03T13:05:08Z

lib/internal/test_runner/reporter/lcov.js

+// https://ltp.sourceforge.net/coverage/lcov/geninfo.1.php
+// Excerpts from this documentation are included in the comments that make up
+// the _transform function below.
+class LcovReporter extends Transform {


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

benjamingr · 2023-10-03T14:31:00Z

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

nodejs-github-bot · 2023-10-03T14:37:57Z

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

benjamingr · 2023-10-03T17:22:32Z

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'

benjamingr · 2023-10-03T17:23:43Z

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

benjamingr · 2023-10-03T17:25:25Z

lib/internal/test_runner/reporter/lcov.js

+        // Then there is a list of execution counts for each instrumented line
+        // (i.e. a line which resulted in executable code):
+        // ## DA:\<line number\>,\<execution count\>[,\<checksum\>]
+        const sortedLines = [...file.lines].sort((a, b) => a.line - b.line);


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`);

});

lib/internal/test_runner/reporter/lcov.js

atlowChemi · 2023-10-03T19:06:59Z

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 
        
           };

atlowChemi · 2023-10-04T03:32:37Z

test/parallel/test-runner-output.mjs

-  for (const { name, fn } of tests) {
-    it(name, fn);
+  for (const { name, fn, options } of tests) {
+    it(name, fn, options);


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.

nodejs-github-bot · 2023-10-04T16:00:24Z

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

philnash · 2023-10-04T23:42:53Z

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.

nodejs-github-bot · 2023-10-25T12:11:34Z

Landed in c60c11a

targos · 2023-11-13T07:30:09Z

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

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

lib/internal/test_runner/reporter/lcov.js Outdated Show resolved Hide resolved

test/fixtures/test-runner/output/lcov_reporter.js Outdated Show resolved Hide resolved

atlowChemi reviewed Oct 3, 2023

View reviewed changes

lib/test/reporters.js Outdated Show resolved Hide resolved

doc/api/test.md Show resolved Hide resolved

lib/internal/test_runner/reporter/lcov.js Outdated Show resolved Hide resolved

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

lib/internal/test_runner/reporter/lcov.js Outdated Show resolved Hide resolved

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

lib/internal/test_runner/reporter/lcov.js Show resolved Hide resolved

lib/internal/test_runner/reporter/lcov.js Outdated Show resolved Hide resolved

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

lib/internal/test_runner/reporter/lcov.js Show resolved Hide resolved

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 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 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`);
	});

Uh oh!

test_runner: adds built in lcov test reporter #50018

test_runner: adds built in lcov test reporter #50018

Uh oh!

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

atlowChemi Oct 4, 2023 •

edited

Loading

atlowChemi Oct 4, 2023 •

edited

Loading