tools: improve `node:test` output when running in github actions by MoLow · Pull Request #49129 · nodejs/node

MoLow · 2023-08-13T07:06:00Z

nodejs-github-bot · 2023-08-13T07:06:04Z

Review requested:

@nodejs/actions
@nodejs/security-wg

atlowChemi · 2023-08-13T14:01:59Z

@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
..................................................

MoLow · 2023-08-13T14:23:13Z

This seems to have not worked?

@atlowChemi this will only change the behavior in case of failed tests

cjihrig · 2023-08-13T18:00:24Z

.github/workflows/build-tarball.yml

        run: |
          cd $TAR_DIR
-          make run-ci -j2 V=1 TEST_CI_ARGS="-p dots --measure-flakiness 9"
+          make run-ci -j2 V=1 TEST_CI_ARGS="-p dots --node-args='--test-reporter=spec' --measure-flakiness 9"


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.

I guess we can do that in a follow-up. perhaps only in the asan job

cjihrig · 2023-08-13T18:02:58Z

.github/workflows/build-tarball.yml

        run: |
          cd $TAR_DIR
-          make run-ci -j2 V=1 TEST_CI_ARGS="-p dots --measure-flakiness 9"
+          make run-ci -j2 V=1 TEST_CI_ARGS="-p dots --node-args='--test-reporter=spec' --measure-flakiness 9"


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?

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

GeoffreyBooth

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.

GeoffreyBooth · 2023-08-14T03:43:39Z

tools/test.py

+  def AboutToRun(self, case):
+    if not hasattr(case, 'additional_flags'):
+      case.additional_flags = []
+    case.additional_flags.append('--test-reporter=./tools/github_reporter/index.js')
+    case.additional_flags.append('--test-reporter-destination=stdout')


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?

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

aduh95 · 2023-08-14T06:46:41Z

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 · 2023-08-14T07:30:30Z

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

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

nodejs-github-bot · 2023-08-14T11:00:01Z

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

nodejs-github-bot · 2023-08-15T07:16:55Z

Landed in 802c52f...9cc7327

PR-URL: #49129 Refs: #49120 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

PR-URL: nodejs#49129 Refs: nodejs#49120 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

PR-URL: #49129 Refs: #49120 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

MoLow requested review from GeoffreyBooth and cjihrig August 13, 2023 07:06

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 force-pushed the improve-output branch from 4685350 to 4477def Compare August 13, 2023 10:18

cjihrig approved these changes Aug 13, 2023

View reviewed changes

GeoffreyBooth reviewed Aug 14, 2023

View reviewed changes

MoLow added 3 commits August 14, 2023 10:25

tools: add @reporters/github to tools

697fe62

tools: use @reporters/github when running in github actions

c81e712

tools: use spec reporter in actions

207f464

MoLow force-pushed the improve-output branch from 4477def to 207f464 Compare August 14, 2023 07:25

aduh95 approved these changes Aug 14, 2023

View reviewed changes

MoLow added request-ci Add this label to start a Jenkins CI on a PR. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. labels Aug 14, 2023

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

MoLow 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 Aug 14, 2023

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 15, 2023

nodejs-github-bot closed this Aug 15, 2023

nodejs-github-bot pushed a commit that referenced this pull request Aug 15, 2023

tools: add @reporters/github to tools

8972f9b

PR-URL: #49129 Refs: #49120 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

nodejs-github-bot pushed a commit that referenced this pull request Aug 15, 2023

tools: use @reporters/github when running in github actions

0d6cdb2

PR-URL: #49129 Refs: #49120 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

nodejs-github-bot pushed a commit that referenced this pull request Aug 15, 2023

tools: use spec reporter in actions

9cc7327

PR-URL: #49129 Refs: #49120 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

MoLow deleted the improve-output branch August 15, 2023 07:45

RafaelGSS pushed a commit that referenced this pull request Aug 15, 2023

tools: add @reporters/github to tools

8075c3b

PR-URL: #49129 Refs: #49120 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

RafaelGSS pushed a commit that referenced this pull request Aug 15, 2023

tools: use @reporters/github when running in github actions

73e0803

PR-URL: #49129 Refs: #49120 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

RafaelGSS pushed a commit that referenced this pull request Aug 15, 2023

tools: use spec reporter in actions

1debb9e

PR-URL: #49129 Refs: #49120 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

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

tools: add @reporters/github to tools

f43e5e1

PR-URL: nodejs#49129 Refs: nodejs#49120 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

RafaelGSS pushed a commit to RafaelGSS/node that referenced this pull request Aug 15, 2023

tools: use spec reporter in actions

350d3d3

PR-URL: nodejs#49129 Refs: nodejs#49120 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

rluvaton pushed a commit to rluvaton/node that referenced this pull request Aug 15, 2023

tools: add @reporters/github to tools

a943887

PR-URL: nodejs#49129 Refs: nodejs#49120 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

rluvaton pushed a commit to rluvaton/node that referenced this pull request Aug 15, 2023

tools: use spec reporter in actions

55deb3b

PR-URL: nodejs#49129 Refs: nodejs#49120 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

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

tools: add @reporters/github to tools

4db70fa

PR-URL: #49129 Refs: #49120 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

RafaelGSS pushed a commit that referenced this pull request Aug 16, 2023

tools: use @reporters/github when running in github actions

e625222

PR-URL: #49129 Refs: #49120 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

RafaelGSS pushed a commit that referenced this pull request Aug 16, 2023

tools: use spec reporter in actions

858d6de

PR-URL: #49129 Refs: #49120 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

RafaelGSS pushed a commit that referenced this pull request Aug 17, 2023

tools: add @reporters/github to tools

95a6e76

PR-URL: #49129 Refs: #49120 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

RafaelGSS pushed a commit that referenced this pull request Aug 17, 2023

tools: use @reporters/github when running in github actions

3471758

PR-URL: #49129 Refs: #49120 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

RafaelGSS pushed a commit that referenced this pull request Aug 17, 2023

tools: use spec reporter in actions

21dc844

PR-URL: #49129 Refs: #49120 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

Uh oh!

Conversation

MoLow commented Aug 13, 2023

Uh oh!

nodejs-github-bot commented Aug 13, 2023

Uh oh!

atlowChemi commented Aug 13, 2023

Uh oh!

MoLow commented Aug 13, 2023

Uh oh!

cjihrig Aug 13, 2023

Choose a reason for hiding this comment

Uh oh!

MoLow Aug 14, 2023

Choose a reason for hiding this comment

Uh oh!

cjihrig Aug 13, 2023

Choose a reason for hiding this comment

Uh oh!

MoLow Aug 14, 2023

Choose a reason for hiding this comment

Uh oh!

GeoffreyBooth left a comment

Choose a reason for hiding this comment

Uh oh!

GeoffreyBooth Aug 14, 2023

Choose a reason for hiding this comment

Uh oh!

MoLow Aug 14, 2023

Choose a reason for hiding this comment

Uh oh!

aduh95 commented Aug 14, 2023

Uh oh!

MoLow commented Aug 14, 2023

Uh oh!

aduh95 left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Aug 14, 2023

Uh oh!

nodejs-github-bot commented Aug 15, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants