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

test: v8: Add test-linux-perf-logger test suite #50352

Merged
merged 2 commits into from Oct 25, 2023

Conversation

lukealbao
Copy link
Contributor

This patch adds test coverage for the --perf-prof-basic* v8 flags.

This ports the repro from #50225, which is fixed in main as of f6f681b.

Relatedly, but out of scope here: I believe this also enables removal of the flaky perf(1) integration tests reported in #50079. Those tests missed the bug reported in #50225. They also don't strictly need to integrate with perf, as far as I can tell. The integration point (the perf map file) is done out-of-band from the perf recording; it is enabled via the v8 flags. See dso__load_perf_map for details on how it is consumed.

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Oct 23, 2023
@mhdawson
Copy link
Member

@lukealbao so that I understand correctly. This improves on v8-updates/test-linux-perf because it validates the output which is fed into perf instead of needed to run perf itself which is where we had the mismatch problem in #50079?

@lukealbao
Copy link
Contributor Author

👋 @mhdawson that's exactly right.

I had considered simply to extend v8-updates/test-linux-perf with the new repro cases, but I realized that the coverage doesn't offer anything that's not available from the perf map file. I'm hoping to land this test suite in any case, so I didn't want to presume to remove the integration test without further input.

@mhdawson
Copy link
Member

@lukealbao thanks for confirming that makes sense to me.

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@mhdawson mhdawson added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 24, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 24, 2023
@nodejs-github-bot
Copy link
Collaborator

@richardlau
Copy link
Member

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

Test failures are #50373. Being fixed by #50375. Will need a fresh (not resumed) CI run to pick up the change when it lands on main.

@H4ad H4ad added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 24, 2023
@richardlau richardlau added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 24, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 24, 2023
@nodejs-github-bot
Copy link
Collaborator

@H4ad H4ad added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 25, 2023
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Oct 25, 2023
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/50352
✔  Done loading data for nodejs/node/pull/50352
----------------------------------- PR info ------------------------------------
Title      test: v8: Add test-linux-perf-logger test suite (#50352)
Author     Luke Albao  (@lukealbao)
Branch     lukealbao:add-v8-perf-fn-only-test -> nodejs:main
Labels     test, author ready, needs-ci
Commits    2
 - test: v8: Add test-linux-perf-logger test suite
 - Lint fixes
Committers 1
 - Luke Albao 
PR-URL: https://github.com/nodejs/node/pull/50352
Reviewed-By: Michael Dawson 
Reviewed-By: Richard Lau 
Reviewed-By: Vinícius Lourenço Claro Cardoso 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/50352
Reviewed-By: Michael Dawson 
Reviewed-By: Richard Lau 
Reviewed-By: Vinícius Lourenço Claro Cardoso 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Mon, 23 Oct 2023 23:08:06 GMT
   ✔  Approvals: 3
   ✔  - Michael Dawson (@mhdawson) (TSC): https://github.com/nodejs/node/pull/50352#pullrequestreview-1695547920
   ✔  - Richard Lau (@richardlau) (TSC): https://github.com/nodejs/node/pull/50352#pullrequestreview-1695665626
   ✔  - Vinícius Lourenço Claro Cardoso (@H4ad): https://github.com/nodejs/node/pull/50352#pullrequestreview-1695926482
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2023-10-24T22:59:21Z: https://ci.nodejs.org/job/node-test-pull-request/55211/
- Querying data for job/node-test-pull-request/55211/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  No git cherry-pick in progress
   ✔  No git am in progress
   ✔  No git rebase in progress
--------------------------------------------------------------------------------
- Bringing origin/main up to date...
From https://github.com/nodejs/node
 * branch                  main       -> FETCH_HEAD
✔  origin/main is now up-to-date
- Downloading patch for 50352
From https://github.com/nodejs/node
 * branch                  refs/pull/50352/merge -> FETCH_HEAD
✔  Fetched commits as 7d306671cc0d..cb1424536a10
--------------------------------------------------------------------------------
[main 0e34a27150] test: v8: Add test-linux-perf-logger test suite
 Author: Luke Albao 
 Date: Mon Oct 23 15:47:38 2023 -0700
 2 files changed, 165 insertions(+)
 create mode 100644 test/fixtures/linux-perf-logger.js
 create mode 100644 test/v8-updates/test-linux-perf-logger.js
[main a806581e7f] Lint fixes
 Author: Luke Albao 
 Date: Mon Oct 23 16:16:07 2023 -0700
 1 file changed, 4 insertions(+), 4 deletions(-)
   ✔  Patches applied
There are 2 commits in the PR. Attempting autorebase.
Rebasing (2/4)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
test: v8: Add test-linux-perf-logger test suite

PR-URL: #50352
Reviewed-By: Michael Dawson midawson@redhat.com
Reviewed-By: Richard Lau rlau@redhat.com
Reviewed-By: Vinícius Lourenço Claro Cardoso contact@viniciusl.com.br

[detached HEAD eb140c5abb] test: v8: Add test-linux-perf-logger test suite
Author: Luke Albao lukealbao@gmail.com
Date: Mon Oct 23 15:47:38 2023 -0700
2 files changed, 165 insertions(+)
create mode 100644 test/fixtures/linux-perf-logger.js
create mode 100644 test/v8-updates/test-linux-perf-logger.js
Rebasing (3/4)
Rebasing (4/4)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
Lint fixes

PR-URL: #50352
Reviewed-By: Michael Dawson midawson@redhat.com
Reviewed-By: Richard Lau rlau@redhat.com
Reviewed-By: Vinícius Lourenço Claro Cardoso contact@viniciusl.com.br

[detached HEAD 29be5edea2] Lint fixes
Author: Luke Albao lukealbao@gmail.com
Date: Mon Oct 23 16:16:07 2023 -0700
1 file changed, 4 insertions(+), 4 deletions(-)

Successfully rebased and updated refs/heads/main.

ℹ Add commit-queue-squash label to land the PR as one commit, or commit-queue-rebase to land as separate commits.

https://github.com/nodejs/node/actions/runs/6647208045

@H4ad H4ad added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Oct 25, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 25, 2023
@nodejs-github-bot nodejs-github-bot merged commit 9c714d8 into nodejs:main Oct 25, 2023
68 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 9c714d8

@lukealbao lukealbao deleted the add-v8-perf-fn-only-test branch October 25, 2023 23:40
alexfernandez pushed a commit to alexfernandez/node that referenced this pull request Nov 1, 2023
PR-URL: nodejs#50352
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
targos pushed a commit that referenced this pull request Nov 11, 2023
PR-URL: #50352
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
UlisesGascon pushed a commit that referenced this pull request Dec 11, 2023
PR-URL: #50352
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
@UlisesGascon UlisesGascon mentioned this pull request Dec 12, 2023
@targos
Copy link
Member

targos commented Dec 29, 2023

This test wasn't run by CI (only V8 CI runs v8-updates tests) but it would have failed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. needs-ci PRs that need a full CI run. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants