test: making test-runner-run.mjs ignore colors #54552

puskin94 · 2024-08-25T14:48:04Z

nodejs-github-bot · 2024-08-25T14:48:09Z

Review requested:

@nodejs/test_runner

RedYetiDev · 2024-08-25T15:29:17Z

Hi! Could you amend your commit message to begin with an active verb? Something like test: ignore colors in `test-runner-run`

puskin94 · 2024-08-25T15:40:16Z

Hi! Could you amend your commit message to begin with an active verb? Something like test: ignore colors in `test-runner-run`

done 😄

codecov · 2024-08-25T17:11:52Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.89%. Comparing base (26b03c1) to head (120d49b).
Report is 27 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #54552      +/-   ##
==========================================
- Coverage   87.90%   87.89%   -0.01%     
==========================================
  Files         651      651              
  Lines      183364   183364              
  Branches    35719    35711       -8     
==========================================
- Hits       161182   161174       -8     
- Misses      15461    15465       +4     
- Partials     6721     6725       +4

see 25 files with indirect coverage changes

puskin94 · 2024-08-25T20:35:10Z

I guess this commit broke the tests I wrote just a couple of days ago and that now are in the main branch: 52322aa

Am I reading this wrong or that commit is actually causing an issue?

Why should I not be allowed to listen on ipv6_link_local which could be locally resolved to a valid IP ?

RedYetiDev · 2024-08-25T20:51:06Z

See #54556

jazelly · 2024-08-26T06:06:37Z

@puskin94 After looking at the previous implementation, server.listen accepts any string passed into dns.lookup. I started thinking introducing this enforcement might be too restricted. I think it's worth it to discuss more on this. I'll raise it to the original issue thread.

puskin94 · 2024-08-26T06:09:33Z

@puskin94 After looking at the previous implementation, server.listen accepts any string passed into dns.lookup. I started thinking introducing this enforcement might be too restricted. I think it's worth it to discuss more on this. I'll raise it to the original issue thread.

@jazelly exactly what I am thinking too. I should be allowed to have whatever_i_want in my /etc/hosts file and being able to listen to it, always imho 😄

MoLow · 2024-08-26T19:42:55Z

This should land only if there are other tests that ensures colors work as expected

puskin94 · 2024-08-27T07:05:54Z

This should land only if there are other tests that ensures colors work as expected

@MoLow went for a more elegant solution then :) just checking for the colors when in TTY

cjihrig · 2024-08-27T13:23:08Z

test/parallel/test-runner-run.mjs

-      '\n',
-    ]);
+
+    if (process.stderr.isTTY) {


What if instead of checking isTTY, you strip the problematic characters all the time.

@cjihrig because is far as I understand we don't want to change the code implementation, we want to keep the colors and we just want to make the test ignore them.. or? am I misinterpreting this?

I meant use util.stripVTControlCharacters() unconditionally in the test.

oh! because I didn't know about its existence 😄 thanks!

jasnell · 2024-08-27T15:25:17Z

test/parallel/test-runner-run.mjs

@@ -4,6 +4,7 @@ import { join } from 'node:path';
 import { describe, it, run } from 'node:test';
 import { dot, spec, tap } from 'node:test/reporters';
 import assert from 'node:assert';
+import utils from 'internal/util/inspect';


I believe this needs the following at the top for the new internal import to work:

// Flags: --expose-internals

yeah I just figured it out, I should have read this comment earlier 😄 I thought it was there in other tests just as a reference, I didn't know it was actually functional...

You could also use the function exported from the public API.

yeah even using the Flags comment didn't work, let's try with the public API this time

mcollina

lgtm

aduh95 · 2024-09-06T18:24:24Z

test/parallel/test-runner-run.mjs

+    assert.strictEqual(result.length, 2);
+    assert.strictEqual(util.stripVTControlCharacters(result[0]), '.');
+    assert.strictEqual(result[1], '\n');


nit: using deepStrictEqual makes for a much better DX if the test was to fail.

Suggested change

assert.strictEqual(result.length, 2);

assert.strictEqual(util.stripVTControlCharacters(result[0]), '.');

assert.strictEqual(result[1], '\n');

assert.deepStrictEqual(

result?.map?.((v, i) => i === 0 ? util.stripVTControlCharacters(v) : v),

['.', '\n'],

);

@aduh95 I am 👎 on this. For 2 lines a loop is just overkill and introduces complexity and confusion IMHO

cjihrig · 2024-09-10T13:12:11Z

It may be worth rebasing this to pick up some of the tests marked as flaky.

Fixes: nodejs#54551

github-actions · 2024-09-10T16:48:53Z

Failed to start CI

   ⚠  Something was pushed to the Pull Request branch since the last approving review.
   ℹ  request-ci label was added by a Collaborator after the last push event.
- Validating Jenkins credentials
✔  Jenkins credentials valid
- Starting PR CI job
✘  Failed to start PR CI: 400 Bad Request

https://github.com/nodejs/node/actions/runs/10797001710

nodejs-github-bot · 2024-09-11T12:28:57Z

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

nodejs-github-bot · 2024-09-12T17:26:53Z

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

nodejs-github-bot · 2024-09-12T19:51:17Z

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

nodejs-github-bot · 2024-09-12T22:02:42Z

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

nodejs-github-bot · 2024-09-12T23:13:05Z

Landed in 92ca0b7

Fixes: #54551 PR-URL: #54552 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Aug 25, 2024

puskin94 force-pushed the test-make-test-runner-run-ignore-colors branch 2 times, most recently from d1ce27f to 1eee01c Compare August 25, 2024 15:39

cjihrig added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 25, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 25, 2024

This comment was marked as outdated.

Sign in to view

puskin94 force-pushed the test-make-test-runner-run-ignore-colors branch 2 times, most recently from 1eee01c to fbff99f Compare August 26, 2024 06:50

cjihrig added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 26, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 26, 2024

This comment was marked as outdated.

Sign in to view

puskin94 force-pushed the test-make-test-runner-run-ignore-colors branch from fbff99f to 79e477d Compare August 27, 2024 07:05

cjihrig reviewed Aug 27, 2024

View reviewed changes

puskin94 force-pushed the test-make-test-runner-run-ignore-colors branch from 79e477d to 3078f79 Compare August 27, 2024 14:19

cjihrig approved these changes Aug 27, 2024

View reviewed changes

jasnell reviewed Aug 27, 2024

View reviewed changes

puskin94 force-pushed the test-make-test-runner-run-ignore-colors branch 2 times, most recently from 7d9ab97 to 09dd583 Compare August 27, 2024 17:03

mcollina approved these changes Aug 31, 2024

View reviewed changes

This comment was marked as outdated.

Sign in to view

aduh95 reviewed Sep 6, 2024

View reviewed changes

This comment was marked as outdated.

Sign in to view

puskin94 force-pushed the test-make-test-runner-run-ignore-colors branch 2 times, most recently from f4e1232 to 21b839f Compare September 10, 2024 14:00

test: strip color chars in test-runner-run

120d49b

Fixes: nodejs#54551

puskin94 force-pushed the test-make-test-runner-run-ignore-colors branch from 21b839f to 120d49b Compare September 10, 2024 14:05

cjihrig added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 10, 2024

github-actions bot added request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Sep 10, 2024

cjihrig removed needs-ci PRs that need a full CI run. request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. labels Sep 12, 2024

cjihrig approved these changes Sep 12, 2024

View reviewed changes

cjihrig added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. labels Sep 12, 2024

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 12, 2024

nodejs-github-bot merged commit 92ca0b7 into nodejs:main Sep 12, 2024
59 checks passed

RafaelGSS pushed a commit that referenced this pull request Sep 16, 2024

test: strip color chars in test-runner-run

0647853

Fixes: #54551 PR-URL: #54552 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

RafaelGSS mentioned this pull request Sep 16, 2024

v22.9.0 proposal #54966

Merged

RafaelGSS pushed a commit that referenced this pull request Sep 16, 2024

test: strip color chars in test-runner-run

430b7a0

Fixes: #54551 PR-URL: #54552 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

RafaelGSS pushed a commit that referenced this pull request Sep 17, 2024

test: strip color chars in test-runner-run

ee385d6

Fixes: #54551 PR-URL: #54552 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: making test-runner-run.mjs ignore colors #54552

test: making test-runner-run.mjs ignore colors #54552

puskin94 commented Aug 25, 2024

nodejs-github-bot commented Aug 25, 2024

RedYetiDev commented Aug 25, 2024

puskin94 commented Aug 25, 2024

codecov bot commented Aug 25, 2024 •

edited

Loading

This comment was marked as outdated.

puskin94 commented Aug 25, 2024 •

edited

Loading

RedYetiDev commented Aug 25, 2024

jazelly commented Aug 26, 2024

puskin94 commented Aug 26, 2024

This comment was marked as outdated.

MoLow commented Aug 26, 2024

puskin94 commented Aug 27, 2024

cjihrig Aug 27, 2024

puskin94 Aug 27, 2024 •

edited

Loading

cjihrig Aug 27, 2024

puskin94 Aug 27, 2024

jasnell Aug 27, 2024

puskin94 Aug 27, 2024

cjihrig Aug 27, 2024

puskin94 Aug 27, 2024

mcollina left a comment

This comment was marked as outdated.

This comment was marked as outdated.

aduh95 Sep 6, 2024 •

edited

Loading

puskin94 Sep 6, 2024 •

edited

Loading

This comment was marked as outdated.

This comment was marked as outdated.

cjihrig commented Sep 10, 2024

github-actions bot commented Sep 10, 2024

nodejs-github-bot commented Sep 11, 2024

nodejs-github-bot commented Sep 12, 2024

nodejs-github-bot commented Sep 12, 2024

nodejs-github-bot commented Sep 12, 2024

nodejs-github-bot commented Sep 12, 2024

test: making test-runner-run.mjs ignore colors #54552

test: making test-runner-run.mjs ignore colors #54552

Conversation

puskin94 commented Aug 25, 2024

nodejs-github-bot commented Aug 25, 2024

RedYetiDev commented Aug 25, 2024

puskin94 commented Aug 25, 2024

codecov bot commented Aug 25, 2024 • edited Loading

Codecov Report

This comment was marked as outdated.

puskin94 commented Aug 25, 2024 • edited Loading

RedYetiDev commented Aug 25, 2024

jazelly commented Aug 26, 2024

puskin94 commented Aug 26, 2024

This comment was marked as outdated.

MoLow commented Aug 26, 2024

puskin94 commented Aug 27, 2024

cjihrig Aug 27, 2024

Choose a reason for hiding this comment

puskin94 Aug 27, 2024 • edited Loading

Choose a reason for hiding this comment

cjihrig Aug 27, 2024

Choose a reason for hiding this comment

puskin94 Aug 27, 2024

Choose a reason for hiding this comment

jasnell Aug 27, 2024

Choose a reason for hiding this comment

puskin94 Aug 27, 2024

Choose a reason for hiding this comment

cjihrig Aug 27, 2024

Choose a reason for hiding this comment

puskin94 Aug 27, 2024

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

This comment was marked as outdated.

This comment was marked as outdated.

aduh95 Sep 6, 2024 • edited Loading

Choose a reason for hiding this comment

puskin94 Sep 6, 2024 • edited Loading

Choose a reason for hiding this comment

This comment was marked as outdated.

This comment was marked as outdated.

cjihrig commented Sep 10, 2024

github-actions bot commented Sep 10, 2024

nodejs-github-bot commented Sep 11, 2024

nodejs-github-bot commented Sep 12, 2024

nodejs-github-bot commented Sep 12, 2024

nodejs-github-bot commented Sep 12, 2024

nodejs-github-bot commented Sep 12, 2024

codecov bot commented Aug 25, 2024 •

edited

Loading

puskin94 commented Aug 25, 2024 •

edited

Loading

puskin94 Aug 27, 2024 •

edited

Loading

aduh95 Sep 6, 2024 •

edited

Loading

puskin94 Sep 6, 2024 •

edited

Loading