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.

Sign up for GitHub

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

Jump to bottom

test_runner: adding colors to the xs and dots in the dot test runner #53450

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

test_runner: adding colors to the xs and dots in the dot test runner #53450

wants to merge 2 commits into from
+5 −5

Conversation

puskin94
Copy link

Fixes: #51770

Making sure that, when executing the dot test reporter, dots are white and Xs are red, for better visibility

@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 labels Jun 14, 2024
@puskin94 puskin94 changed the title lib: adding colors to the xs and dots in the test runner lib: adding colors to the xs and dots in the dot test runner Jun 14, 2024
MoLow
MoLow reviewed Jun 14, 2024
View reviewed changes
lib/internal/test_runner/reporter/dot.js Outdated
const { formatTestReport } = require('internal/test_runner/reporter/utils');
const { red, white, green, clear } = require('internal/util/colors');
Copy link
Member

Choose a reason for hiding this comment

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

please avoid destructuring here so colors.refresh will still work

Copy link
Author

Choose a reason for hiding this comment

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

@MoLow thanks for the super prompt response :)

I applied the code change, but I don't see any reference to colors.refresh, can you pinpoint me to the right place?

@puskin94 puskin94 requested a review from MoLow June 14, 2024 09:59
@RedYetiDev
Copy link
Member

IIRC isn't there a PR already open doing this?

Maybe one or the other should be closed in favor?

@puskin94
Copy link
Author

IIRC isn't there a PR already open doing this?

Maybe one or the other should be closed in favor?

yeah I noticed, but I pushed mine forward anyway because the creator or the other PR didn't push in the last 2 months

@RedYetiDev
Copy link
Member

RedYetiDev commented Jun 14, 2024 •
edited
Loading

No problem! If this gets merged, I'll close the other one.

@RedYetiDev RedYetiDev mentioned this pull request Jun 14, 2024
Closed
atlowChemi
atlowChemi reviewed Jun 14, 2024
View reviewed changes
lib/internal/test_runner/reporter/dot.js Outdated
const { formatTestReport } = require('internal/test_runner/reporter/utils');
const colors = require('internal/util/colors');
Copy link
Member

@atlowChemi atlowChemi Jun 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This seems like an unrelated change, could you revert it? (The import existed already in the line above)

Copy link
Author

Choose a reason for hiding this comment

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

@atlowChemi done!

@puskin94 puskin94 requested a review from atlowChemi June 14, 2024 15:22
@jakecastelli
Copy link
Contributor

jakecastelli commented Jun 14, 2024 •
edited
Loading

Good work on pushing the progress! (Just a thought - I think the origin author might deserve a co-author in the commit message? As the change looks quite identical).

@atlowChemi
Copy link
Member

@puskin94 the PR changes the test_runner submodule, so the first commit message should be prefixed with test_runner and not lib.
Could you please rebase and fix the message? Let me know if you need assistance with that 🙂

@puskin94 puskin94 force-pushed the make-dot-and-x-test-runner-colored branch 4 times, most recently from 8c23d44 to 73885f8 Compare June 15, 2024 08:24
@puskin94 puskin94 force-pushed the make-dot-and-x-test-runner-colored branch from 73885f8 to 68aa96e Compare June 15, 2024 08:26
@puskin94
Copy link
Author

@puskin94 the PR changes the test_runner submodule, so the first commit message should be prefixed with test_runner and not lib. Could you please rebase and fix the message? Let me know if you need assistance with that 🙂

@atlowChemi it took me a bit to understand the right order of execution of the commands, but we should be there now :) Thanks!

@atlowChemi atlowChemi added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 15, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 15, 2024
@nodejs-github-bot
Copy link
Collaborator

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

@puskin94 puskin94 changed the title lib: adding colors to the xs and dots in the dot test runner test_runner: adding colors to the xs and dots in the dot test runner Jun 16, 2024
@puskin94
Copy link
Author

Can anyone help me out here? I see the PR is stuck and I guess it is because of broken jenkins tests...
I tried to reproduce them but everything is passing locally.
What should I do? How can I move?

Thanks in advance!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lpinca lpinca lpinca approved these changes

@MoLow MoLow MoLow approved these changes

@atlowChemi atlowChemi atlowChemi approved these changes

Assignees
No one assigned
Labels
needs-ci PRs that need a full CI run. test_runner
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

test_runner: make the dot reporter colored
7 participants
@puskin94 @nodejs-github-bot @RedYetiDev @jakecastelli @atlowChemi @lpinca @MoLow