Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

display of test_runner table #51351

Closed
wants to merge 0 commits into from
Closed

Conversation

Medhansh404
Copy link

@Medhansh404 Medhansh404 commented Jan 3, 2024

The issue #51299 about the incomplete file names should be addressed by these changes in the PR

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/test_runner

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem. labels Jan 3, 2024
@benjamingr
Copy link
Member

Thanks! Please add a test and update existing ones for coverage table output

@Medhansh404
Copy link
Author

I'm sorry but I have not made formal tests supporting a PR before this, this is kinda embarrasing but i am eager to learn, can you guide me so that i can learn doing it like in a formal way. thanks in advanvce :)

@pulkit-30
Copy link
Contributor

Hey @Medhansh404, you can take reference from test/parallel/test-runner-coverage.js, and don't be embarrassed for asking help.

@MoLow
Copy link
Member

MoLow commented Jan 4, 2024

I'm sorry but I have not made formal tests supporting a PR before this, this is kinda embarrasing but i am eager to learn, can you guide me so that i can learn doing it like in a formal way. thanks in advanvce :)

its totally ok. I beileve this code is covered by some tests, I ran the tests for this PR and I believe they would fail

Copy link
Member

@atlowChemi atlowChemi left a comment

Choose a reason for hiding this comment

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

Overall LGTM, please use primordials

lib/internal/test_runner/utils.js Outdated Show resolved Hide resolved
lib/internal/test_runner/utils.js Outdated Show resolved Hide resolved
lib/internal/test_runner/utils.js Outdated Show resolved Hide resolved
lib/internal/test_runner/utils.js Outdated Show resolved Hide resolved
lib/internal/test_runner/utils.js Outdated Show resolved Hide resolved
@MoLow
Copy link
Member

MoLow commented Jan 6, 2024

@Medhansh404 please remove merge commits.

function getCell(string, width, pad, truncate, coverage) {
if (!table) return string;

let result = string;
if (pad) result = pad(result, width);
if (truncate) result = truncate(result, width);
Copy link
Member

Choose a reason for hiding this comment

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

isnt this still in use in the uncoverd lines cell?

Copy link
Author

Choose a reason for hiding this comment

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

The suggested pad change was passed as an argument in the function already similar to the getCell function here, still I committed the changes suggested, for this I have added the truncating line again.

@atlowChemi
Copy link
Member

Nice work @Medhansh404!
In order for the CI to work properly, the branch can't have merge commits (such as 2050255 and 501c311)
Please rebase the branch so we could start the CI, let me know if you need any help with that 🙂

const lines = [];
let currentLine = '';

for (const word of StringPrototypeSplit(string, '\\')) {
Copy link
Member

Choose a reason for hiding this comment

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

this should probably use ArrayPrototypeForEach instead of for..of

Copy link
Contributor

@aduh95 aduh95 Jan 8, 2024

Choose a reason for hiding this comment

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

Or alternatively, a simple classic for loop

### Unsafe array iteration
There are many usual practices in JavaScript that rely on iteration. It's useful
to be aware of them when dealing with arrays (or `TypedArray`s) in core as array
iteration typically calls several user-mutable methods. This sections lists the
most common patterns in which ECMAScript code relies non-explicitly on array
iteration and how to avoid it.
<details>
<summary>Avoid for-of loops on arrays</summary>
```js
for (const item of array) {
console.log(item);
}
```
This code is internally expanded into something that looks like:
```js
{
// 1. Lookup @@iterator property on `array` (user-mutable if user-provided).
// 2. Lookup @@iterator property on %Array.prototype% (user-mutable).
// 3. Call that function.
const iterator = array[Symbol.iterator]();
// 1. Lookup `next` property on `iterator` (doesn't exist).
// 2. Lookup `next` property on %ArrayIteratorPrototype% (user-mutable).
// 3. Call that function.
let { done, value: item } = iterator.next();
while (!done) {
console.log(item);
// Repeat.
({ done, value: item } = iterator.next());
}
}
```
Instead of utilizing iterators, you can use the more traditional but still very
performant `for` loop:
```js
for (let i = 0; i < array.length; i++) {
console.log(array[i]);
}
```
The following code snippet illustrates how user-land code could impact the
behavior of internal modules:
```js
// User-land
Array.prototype[Symbol.iterator] = () => ({
next: () => ({ done: true }),
});
// Core
let forOfLoopBlockExecuted = false;
let forLoopBlockExecuted = false;
const array = [1, 2, 3];
for (const item of array) {
forOfLoopBlockExecuted = true;
}
for (let i = 0; i < array.length; i++) {
forLoopBlockExecuted = true;
}
console.log(forOfLoopBlockExecuted); // false
console.log(forLoopBlockExecuted); // true
```

Copy link
Author

Choose a reason for hiding this comment

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

Thanks @aduh95 i'll probably stick to the classic loop of the format for (let i = 0; i < array.length; i++) { console.log(array[i]); } thanks for the reference !! :)

@@ -403,7 +438,7 @@ function getCoverageReport(pad, summary, symbol, color, table) {
});
fileCoverage /= kColumnsKeys.length;

report += `${prefix}${getCell(relativePath, filePadLength, StringPrototypePadEnd, truncateStart, fileCoverage)}${kSeparator}` +
report += `${prefix}${getfilePathMultiline(relativePath, filePadLength, StringPrototypePadEnd, fileCoverage)}${kSeparator}` +
Copy link
Member

Choose a reason for hiding this comment

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

pad should now be a boolean here, not a function

@MoLow
Copy link
Member

MoLow commented Jan 10, 2024

In general, LGTM. please squash/remove the merge commits

@Medhansh404
Copy link
Author

Sorry for the inconvenience, i was learning about git squash and merge command and i made changes in the main branch. Should I close the PR or is there a way to revert back?

@aduh95
Copy link
Contributor

aduh95 commented Jan 23, 2024

You don't need to close the PR, here's the quickest way to fix your branch:

git fetch https://github.com/nodejs/node.git HEAD
git reset FETCH_HEAD --soft
git commit
git push --force-with-lease

This will create a single new commit on top of main, which you can then force push to this branch (which will erase all the 92 commits that are in it right now).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test_runner: --experimental-test-coverage generates partial file names
8 participants