GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — tools: improve `node:test` output when running in github actions by MoLow · Pull Request #49129 · nodejs/node

Conversation

@MoLow

@MoLow MoLow commented Aug 13, 2023

Copy link
Copy Markdown
Member

Refs: #49120

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
atlowChemi and bricss reacted with rocket emoji
All reactions
  • 🚀 2 reactions
@MoLow
MoLow requested review from GeoffreyBooth and cjihrig August 13, 2023 07:06
@nodejs-github-bot

nodejs-github-bot commented Aug 13, 2023

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/actions
  • @nodejs/security-wg
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot nodejs-github-bot added meta Issues and PRs related to the general management of the project. tools Issues and PRs related to the tools directory. labels Aug 13, 2023
@MoLow
MoLow force-pushed the improve-output branch from 4685350 to 4477def Compare August 13, 2023 10:18
@atlowChemi

atlowChemi commented Aug 13, 2023

Copy link
Copy Markdown
Member

@MoLow This seems to have not worked?
https://github.com/nodejs/node/actions/runs/5846748905/job/15852264716#step:6:5487

/opt/hostedtoolcache/Python/3.11.4/x64/bin/python3 tools/test.py  -p tap --logfile test.tap \
	--mode=release --flaky-tests=keep_retrying \
	-p actions --node-args='--test-reporter=spec' --node-args='--test-reporter-destination=stdout' -t 300 --measure-flakiness 9 default pummel addons js-native-api node-api benchmark 
Running 4010 tests
..................................................
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@MoLow

MoLow commented Aug 13, 2023

Copy link
Copy Markdown
Member Author

This seems to have not worked?

@atlowChemi this will only change the behavior in case of failed tests
atlowChemi reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
cjihrig
cjihrig approved these changes Aug 13, 2023
View reviewed changes
Comment thread .github/workflows/build-tarball.yml Outdated

@cjihrig cjihrig Aug 13, 2023

Copy link
Copy Markdown
Contributor

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.

This should be an improvement in terms of the amount of output we see from failed tests, which should help with debugging.

There is also the issue of the asan CI job truncating output. Since node:test is never used as the main test runner, a possible next step to address that issue could be to create a custom reporter in tools/ that only outputs the test failures and diagnostic messages. It could basically be a slimmed down version of the spec reporter.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@MoLow MoLow Aug 14, 2023

Copy link
Copy Markdown
Member Author

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 guess we can do that in a follow-up. perhaps only in the asan job

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread .github/workflows/build-tarball.yml Outdated

@cjihrig cjihrig Aug 13, 2023

Copy link
Copy Markdown
Contributor

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.

It looks like the CI ran fine so I guess there is no issue, but are we sure this won't impact any of the tests that expect different reporters?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@MoLow MoLow Aug 14, 2023

Copy link
Copy Markdown
Member Author

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.

no issue. tests that look at reporters (e.g snapshot tests, exit code tests etc) always spawn a child and control the env vars and flags

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
GeoffreyBooth
GeoffreyBooth reviewed Aug 14, 2023
View reviewed changes

@GeoffreyBooth GeoffreyBooth left a comment

Copy link
Copy Markdown
Member

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.

Don’t we usually add dependencies in a dedicated PR? Though this seems isolated enough that I wouldn’t object in this case.

It would be nice to see an example of the output of a failed test/job using this PR.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread tools/test.py Outdated
Comment on lines 319 to 323

@GeoffreyBooth GeoffreyBooth Aug 14, 2023

Copy link
Copy Markdown
Member

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.

If I understand this correctly, this is adding two additional flags to all the tests that run. This feels worrisome, because as a test author I’d expect the flags I define at the top of the test file to be the only flags that are used; I can imagine issues with process.argv and so on. Could these be defined via environment variables instead?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@MoLow MoLow Aug 14, 2023

Copy link
Copy Markdown
Member Author

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.

this is only the case in github actions, within jenkins or locally the flags wont be added.
the only way to add this via an environment variable is with NODE_OPTIONS

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@aduh95

aduh95 commented Aug 14, 2023

Copy link
Copy Markdown
Contributor

Adding the node_modules folder is a bit much don't you think? Is there a way to keep only the useful part by e.g. bundling it?
MoLow and atlowChemi reacted with thumbs up emoji MoLow reacted with eyes emoji
All reactions
  • 👍 2 reactions
  • 👀 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
MoLow added 3 commits August 14, 2023 10:25
@MoLow
tools: add @reporters/github to tools
697fe62
@MoLow
tools: use @reporters/github when running in github actions
c81e712
@MoLow
tools: use spec reporter in actions
207f464
@MoLow
MoLow force-pushed the improve-output branch from 4477def to 207f464 Compare August 14, 2023 07:25
@MoLow

MoLow commented Aug 14, 2023

Copy link
Copy Markdown
Member Author

Adding the node_modules folder is a bit much don't you think? Is there a way to keep only the useful part by e.g. bundling it?

done
aduh95 reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
aduh95
aduh95 approved these changes Aug 14, 2023
View reviewed changes

@aduh95 aduh95 left a comment

Copy link
Copy Markdown
Contributor

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.

RSLGTM for tools/github_reporter/index.js, LGTM for the rest :)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@MoLow MoLow added request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. commit-queue-rebase PRs the Commit Queue should land as multiple self-contained commits. labels Aug 14, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Aug 14, 2023
@nodejs-github-bot

nodejs-github-bot commented Aug 14, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/53303/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@MoLow MoLow added author ready PRs with CI started, the required approvals, and no outstanding review comments. commit-queue PRs queued for automated landing through the Commit Queue. labels Aug 14, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Aug 15, 2023
@nodejs-github-bot

nodejs-github-bot commented Aug 15, 2023

Copy link
Copy Markdown
Collaborator

Landed in 802c52f...9cc7327
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot nodejs-github-bot closed this Aug 15, 2023
nodejs-github-bot pushed a commit that referenced this pull request Aug 15, 2023
@MoLow @nodejs-github-bot
tools: add @reporters/github to tools
8972f9b 
PR-URL: #49129
Refs: #49120
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
nodejs-github-bot pushed a commit that referenced this pull request Aug 15, 2023
@MoLow @nodejs-github-bot
tools: use @reporters/github when running in github actions
0d6cdb2 
PR-URL: #49129
Refs: #49120
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
nodejs-github-bot pushed a commit that referenced this pull request Aug 15, 2023
@MoLow @nodejs-github-bot
tools: use spec reporter in actions
9cc7327 
PR-URL: #49129
Refs: #49120
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
@MoLow
MoLow deleted the improve-output branch August 15, 2023 07:45
RafaelGSS pushed a commit that referenced this pull request Aug 15, 2023
@MoLow @RafaelGSS
tools: add @reporters/github to tools
8075c3b 
PR-URL: #49129
Refs: #49120
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
RafaelGSS pushed a commit that referenced this pull request Aug 15, 2023
@MoLow @RafaelGSS
tools: use @reporters/github when running in github actions
73e0803 
PR-URL: #49129
Refs: #49120
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
RafaelGSS pushed a commit that referenced this pull request Aug 15, 2023
@MoLow @RafaelGSS
tools: use spec reporter in actions
1debb9e 
PR-URL: #49129
Refs: #49120
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
@UlisesGascon UlisesGascon mentioned this pull request Aug 15, 2023
v20.6.0 proposal #49185
Merged
RafaelGSS pushed a commit to RafaelGSS/node that referenced this pull request Aug 15, 2023
@MoLow @RafaelGSS
tools: add @reporters/github to tools
f43e5e1 
PR-URL: nodejs#49129
Refs: nodejs#49120
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
RafaelGSS pushed a commit to RafaelGSS/node that referenced this pull request Aug 15, 2023
@MoLow @RafaelGSS
tools: use @reporters/github when running in github actions
9b8f69e 
PR-URL: nodejs#49129
Refs: nodejs#49120
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
RafaelGSS pushed a commit to RafaelGSS/node that referenced this pull request Aug 15, 2023
@MoLow @RafaelGSS
tools: use spec reporter in actions
350d3d3 
PR-URL: nodejs#49129
Refs: nodejs#49120
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
@MoLow MoLow mentioned this pull request Aug 16, 2023
tools: fix github reporter appended multiple times #49199
Merged
RafaelGSS pushed a commit that referenced this pull request Aug 16, 2023
@MoLow @RafaelGSS
tools: add @reporters/github to tools
4db70fa 
PR-URL: #49129
Refs: #49120
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
RafaelGSS pushed a commit that referenced this pull request Aug 16, 2023
@MoLow @RafaelGSS
tools: use @reporters/github when running in github actions
e625222 
PR-URL: #49129
Refs: #49120
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
RafaelGSS pushed a commit that referenced this pull request Aug 16, 2023
@MoLow @RafaelGSS
tools: use spec reporter in actions
858d6de 
PR-URL: #49129
Refs: #49120
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
RafaelGSS pushed a commit that referenced this pull request Aug 17, 2023
@MoLow @RafaelGSS
tools: add @reporters/github to tools
95a6e76 
PR-URL: #49129
Refs: #49120
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
RafaelGSS pushed a commit that referenced this pull request Aug 17, 2023
@MoLow @RafaelGSS
tools: use @reporters/github when running in github actions
3471758 
PR-URL: #49129
Refs: #49120
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
RafaelGSS pushed a commit that referenced this pull request Aug 17, 2023
@MoLow @RafaelGSS
tools: use spec reporter in actions
21dc844 
PR-URL: #49129
Refs: #49120
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@GeoffreyBooth GeoffreyBooth GeoffreyBooth left review comments
@cjihrig cjihrig cjihrig approved these changes
@aduh95 aduh95 aduh95 approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. commit-queue-rebase PRs the Commit Queue should land as multiple self-contained commits. meta Issues and PRs related to the general management of the project. tools Issues and PRs related to the tools directory.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@MoLow @nodejs-github-bot @atlowChemi @aduh95 @GeoffreyBooth @cjihrig
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.