-
Notifications
You must be signed in to change notification settings - Fork 29.1k
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
test: making test-runner-run.mjs ignore colors #54552
test: making test-runner-run.mjs ignore colors #54552
Conversation
Review requested:
|
Hi! Could you amend your commit message to begin with an active verb? Something like |
d1ce27f
to
1eee01c
Compare
done 😄 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
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 |
This comment was marked as outdated.
This comment was marked as outdated.
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 |
See #54556 |
@puskin94 After looking at the previous implementation, |
@jazelly exactly what I am thinking too. I should be allowed to have |
1eee01c
to
fbff99f
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This should land only if there are other tests that ensures colors work as expected |
fbff99f
to
79e477d
Compare
@MoLow went for a more elegant solution then :) just checking for the colors when in TTY |
test/parallel/test-runner-run.mjs
Outdated
'\n', | ||
]); | ||
|
||
if (process.stderr.isTTY) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if instead of checking isTTY
, you strip the problematic characters all the time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant use util.stripVTControlCharacters()
unconditionally in the test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh! because I didn't know about its existence 😄 thanks!
79e477d
to
3078f79
Compare
test/parallel/test-runner-run.mjs
Outdated
@@ -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'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this needs the following at the top for the new internal
import to work:
// Flags: --expose-internals
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could also use the function exported from the public API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah even using the Flags
comment didn't work, let's try with the public API this time
7d9ab97
to
09dd583
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
assert.strictEqual(result.length, 2); | ||
assert.strictEqual(util.stripVTControlCharacters(result[0]), '.'); | ||
assert.strictEqual(result[1], '\n'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: using deepStrictEqual
makes for a much better DX if the test was to fail.
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'], | |
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aduh95 I am 👎 on this. For 2 lines a loop is just overkill and introduces complexity and confusion IMHO
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
It may be worth rebasing this to pick up some of the tests marked as flaky. |
f4e1232
to
21b839f
Compare
21b839f
to
120d49b
Compare
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 Requesthttps://github.com/nodejs/node/actions/runs/10797001710 |
Landed in 92ca0b7 |
Fixes: #54551