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 — test: v8: Add test-linux-perf-logger test suite by lukealbao · Pull Request #50352 · nodejs/node

Conversation

@lukealbao

@lukealbao lukealbao commented Oct 23, 2023

Copy link
Copy Markdown
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.

Sorry, something went wrong.

Uh oh!

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

bricss reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@lukealbao
test: v8: Add test-linux-perf-logger test suite
c300bf3
@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to Node.js core tests and test infrastructure. labels Oct 23, 2023
@lukealbao lukealbao mentioned this pull request Oct 23, 2023
v8-updates/test-linux-perf no longer works with recent V8 versions #50079
Closed
@lukealbao
Lint fixes
cb14245
@mhdawson

mhdawson commented Oct 24, 2023

Copy link
Copy Markdown
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?

All reactions

Sorry, something went wrong.

Uh oh!

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

@lukealbao

lukealbao commented Oct 24, 2023

Copy link
Copy Markdown
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.

All reactions

Sorry, something went wrong.

Uh oh!

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

@mhdawson

mhdawson commented Oct 24, 2023

Copy link
Copy Markdown
Member

@lukealbao thanks for confirming that makes sense to me.

All reactions

Sorry, something went wrong.

Uh oh!

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

mhdawson
mhdawson approved these changes Oct 24, 2023
View reviewed changes

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

LGTM

Sorry, something went wrong.

Uh oh!

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

All reactions
@mhdawson mhdawson added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Oct 24, 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 Oct 24, 2023
@nodejs-github-bot

nodejs-github-bot commented Oct 24, 2023

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

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

@richardlau

richardlau commented Oct 24, 2023

Copy link
Copy Markdown
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.

All reactions

Sorry, something went wrong.

Uh oh!

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

richardlau
richardlau approved these changes Oct 24, 2023
View reviewed changes
H4ad
H4ad approved these changes Oct 24, 2023
View reviewed changes
@H4ad H4ad added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Oct 24, 2023
@richardlau richardlau added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Oct 24, 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 Oct 24, 2023
@nodejs-github-bot

nodejs-github-bot commented Oct 24, 2023

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

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

@github-actions github-actions Bot mentioned this pull request Oct 25, 2023
CI Reliability 2023-10-25 nodejs/reliability#697
Open
16 tasks
@H4ad H4ad added the commit-queue PRs queued for automated landing through the Commit Queue. label Oct 25, 2023
@nodejs-github-bot nodejs-github-bot added commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. and removed commit-queue PRs queued for automated landing through the Commit Queue. labels Oct 25, 2023
@nodejs-github-bot

nodejs-github-bot commented Oct 25, 2023

Copy link
Copy Markdown
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 [email protected]
Reviewed-By: Richard Lau [email protected]
Reviewed-By: Vinícius Lourenço Claro Cardoso [email protected]

[detached HEAD eb140c5abb] test: v8: Add test-linux-perf-logger test suite
Author: Luke Albao [email protected]
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 [email protected]
Reviewed-By: Richard Lau [email protected]
Reviewed-By: Vinícius Lourenço Claro Cardoso [email protected]

[detached HEAD 29be5edea2] Lint fixes
Author: Luke Albao [email protected]
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

All reactions

Sorry, something went wrong.

Uh oh!

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

@H4ad H4ad added commit-queue PRs queued for automated landing through the Commit Queue. commit-queue-squash PRs the Commit Queue should land as one squashed commit. and removed commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. labels Oct 25, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Oct 25, 2023
@nodejs-github-bot
nodejs-github-bot merged commit 9c714d8 into nodejs:main Oct 25, 2023
@nodejs-github-bot

nodejs-github-bot commented Oct 25, 2023

Copy link
Copy Markdown
Collaborator

Landed in 9c714d8

All reactions

Sorry, something went wrong.

Uh oh!

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

@lukealbao
lukealbao deleted the add-v8-perf-fn-only-test branch October 25, 2023 23:40
This was referenced Oct 26, 2023
CI Reliability 2023-10-26 nodejs/reliability#698
Open
CI Reliability 2023-10-27 nodejs/reliability#699
Open
CI Reliability 2023-10-28 nodejs/reliability#700
Open
targos pushed a commit that referenced this pull request Nov 11, 2023
@lukealbao @targos
test: v8: Add test-linux-perf-logger test suite
ab9cad8
PR-URL: #50352
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
@targos targos mentioned this pull request Nov 12, 2023
v21.2.0 release proposal #50681
Merged
UlisesGascon pushed a commit that referenced this pull request Dec 11, 2023
@lukealbao @UlisesGascon
test: v8: Add test-linux-perf-logger test suite
a31f9bf
PR-URL: #50352
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
@UlisesGascon UlisesGascon mentioned this pull request Dec 12, 2023
v20.11.0 proposal #51124
Merged
@targos

targos commented Dec 29, 2023

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

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

@targos targos mentioned this pull request Dec 29, 2023
Broken test/v8-updates/test-linux-perf-logger #51308
Closed
@lukealbao lukealbao mentioned this pull request Dec 30, 2023
deps: update V8 to 11.9 #50115
Closed
lukealbao added a commit to lukealbao/node that referenced this pull request May 9, 2024
@lukealbao
test: v8: Add test-linux-perf-logger test suite
7d8fec9
Cherry-picked from 9c714d8

PR-URL: nodejs#50352
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
@lukealbao lukealbao mentioned this pull request May 9, 2024
[v18 backport] test: add v8-updates/test-linux-perf-logger #52925
Closed
richardlau pushed a commit that referenced this pull request May 16, 2024
@lukealbao @richardlau
test: v8: Add test-linux-perf-logger test suite
351ef18
Cherry-picked from 9c714d8

PR-URL: #50352
Backport-PR-URL: #52925
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
@richardlau richardlau mentioned this pull request May 16, 2024
v18.20.3 proposal #53028
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@richardlau richardlau richardlau approved these changes
@H4ad H4ad H4ad approved these changes
+1 more reviewer
@mhdawson mhdawson mhdawson approved these changes
Reviewers whose approvals may not affect merge requirements

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-squash PRs the Commit Queue should land as one squashed commit. needs-ci PRs that need a full CI run. test Issues and PRs related to Node.js core tests and test infrastructure.

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

@lukealbao @mhdawson @nodejs-github-bot @richardlau @targos @H4ad
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.