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.

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

Linux perf tests are not running for node-test-commit-v8-linux #1774

Closed
mmarchini opened this issue Apr 16, 2019 · 13 comments
Closed

Linux perf tests are not running for node-test-commit-v8-linux #1774

mmarchini opened this issue Apr 16, 2019 · 13 comments

Comments

@mmarchini
Copy link
Contributor

Related to #1274. Turns out Linux perf is not installed on machines it should be to run Linux perf tests. I'm not sure for how long, but I'm guessing it has been for a few months, since recent versions of V8 have introduced some changes which would make the test fail (which are fixed by nodejs/node#27265).

Today the Linux perf test is very lenient: if it doesn't find the perf binary or if there's any issue while running perf, the test will skip. Can we somehow force it to fail only on the jobs it's supposed to run (node-test-commit-v8-linux)?

@joyeecheung
Copy link
Member

joyeecheung commented Apr 17, 2019

I think we could use an environment variable to force the test?

@refack
Copy link
Contributor

refack commented Apr 18, 2019

Since it's run only as part of https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=benchmark,v8test=v8test/ I'd think it can always fail.

@refack
Copy link
Contributor

refack commented Apr 18, 2019

Meanwhile I changed the condition so that the v8-update suite will run on benchmark-ubuntu1604-intel-64 as well.

Refs: #1381

@refack
Copy link
Contributor

refack commented Apr 18, 2019

I had to tweak the conditional a few more times:
https://ci.nodejs.org/job/node-test-commit-v8-linux/2242/ ⚠️

14:43:47 + perf --version
14:43:47 /tmp/jenkins1214291362641239044.sh: 104: /tmp/jenkins1214291362641239044.sh: perf: not found
14:43:47 + exit 54

@refack
Copy link
Contributor

refack commented Apr 18, 2019

This is interesting:

14:39:31 + NODE_TEST_DIR=/home/iojs/node-tmp NODE_COMMON_PORT=15000 python tools/test.py -j 88 -p tap --mode=release --flaky-tests=run v8-updates
14:39:31 TAP version 13
14:39:31 1..2
14:39:32 ok 1 v8-updates/test-linux-perf # skip Failed to execute perf: 
14:39:32   ---
14:39:32   duration_ms: 0.915
14:39:32   ...
14:39:32 ok 2 v8-updates/test-postmortem-metadata
14:39:32   ---
14:39:32   duration_ms: 0.916
14:39:32   ...
14:39:32 + perf --version
14:39:32 perf version 4.17.g7d3bf

@mmarchini
Copy link
Contributor Author

@refack thank you for looking into it. I'm getting this failure on my machine as well, I think this might be caused by nodejs/node#27346. I'll fix the test once the flag is fixed.

Since it's run only as part of ci.nodejs.org/job/node-test-commit-v8-linux/nodes=benchmark,v8test=v8test I'd think it can always fail.

I'm not sure I understood what you mean by "it can always fail".

I think we could use an environment variable to force the test?

Sounds like a good idea.

@refack
Copy link
Contributor

refack commented Apr 22, 2019

Since it's run only as part of ci.nodejs.org/job/node-test-commit-v8-linux/nodes=benchmark,v8test=v8test I'd think it can always fail.

I'm not sure I understood what you mean by "it can always fail".

I mean fail the test if perf isn't installed and if the call fails, i.e. remove all the defenses in https://github.com/nodejs/node/blob/49d3d11ba7975b2c6df4ecab55b4619da46fcb95/test/v8-updates/test-linux-perf.js#L51-L65

I mean this test (as part of the v8-update suite) is only triggered when explicitly called, so I'd assume the user will more likely want to know if it actually passed, then skipped.

@mmarchini
Copy link
Contributor Author

I mean this test (as part of the v8-update suite) is only triggered when explicitly called, so I'd assume the user will more likely want to know if it actually passed, then skipped.

Agreed. We should still skip this test if not running on Linux, but it should fail if it can't run on Linux because of missing perf. Since v8-updates only runs on node-test-commit-v8-linux I guess this is fine.

On a side note: can I be included in the e-mail notifications for https://ci.nodejs.org/view/Node.js%20Daily/job/node-update-v8-canary? I don't have permissions to add myself there.

@targos
Copy link
Member

targos commented Apr 23, 2019

@mmarchini I added your address to the list for e-mail notifications

@refack
Copy link
Contributor

refack commented Apr 23, 2019

Cross-Ref: nodejs/node#27364 (un-skip perf failures)

refack added a commit to refack/node that referenced this issue Jun 6, 2019
Co-authored-by: Matheus Marchini <mat@mmarchini.me>

PR-URL: nodejs#27364
Refs: nodejs/build#1774
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
BridgeAR pushed a commit to nodejs/node that referenced this issue Jun 17, 2019
Co-authored-by: Matheus Marchini <mat@mmarchini.me>

PR-URL: #27364
Refs: nodejs/build#1774
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@github-actions
Copy link

github-actions bot commented Mar 5, 2020

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days

@github-actions github-actions bot added the stale label Mar 5, 2020
@mmarchini mmarchini removed the stale label Mar 5, 2020
@mmarchini
Copy link
Contributor Author

Love the reminder :)

Will double check if this is still an issue before closing.

richardlau pushed a commit to nodejs/node that referenced this issue Oct 9, 2020
Co-authored-by: Matheus Marchini <mat@mmarchini.me>

PR-URL: #27364
Refs: nodejs/build#1774
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@github-actions
Copy link

This issue is stale because it has been open many days with no activity. It will be closed soon unless the stale label is removed or a comment is made.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants