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.

Sign up for GitHub

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

Jump to bottom

test: add --trace-gc flag test #42471

Merged
merged 5 commits into from
Mar 28, 2022
Merged

test: add --trace-gc flag test #42471

merged 5 commits into from
Mar 28, 2022

Conversation

tony-go
Copy link
Member

@tony-go tony-go commented Mar 25, 2022 •
edited
Loading

Hi 👋

Context

This pull request follows this previous one and the initiative started by the diagnostic wg.

Added

Now that --trace-gc is a part of the diagnostic tooling support, I thought that we could improve maturity tiers by adding a simple high-level test. As --trace-gc is a part of the v8 codebase, it probably doesn't make sense to test specific cases here. I just tried to test the integration between node and v8.

I first tried to find where this integration was made, but I didn't find it. While I read the code I expected to find it somewhere around here:

node/src/node_options.cc

Line 238 in de163d5

StringVector* const v8_args,

If someone has an idea about where the integration is made, let me know ^^

How to run this test? -> make test-v8-updates

I wasn't sure about where should the test be or how I should write it, so I try to make it simple. 🙈

@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 Mar 25, 2022
jasnell
jasnell reviewed Mar 25, 2022
View reviewed changes
test/fixtures/gc.js Outdated Show resolved Hide resolved
@tony-go
Copy link
Member Author

tony-go commented Mar 26, 2022

cc @nodejs/diagnostics

RafaelGSS
RafaelGSS approved these changes Mar 26, 2022
View reviewed changes
Copy link
Member

@RafaelGSS RafaelGSS left a comment

Choose a reason for hiding this comment

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

LGTM.

@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@github-actions github-actions bot mentioned this pull request Mar 27, 2022
Open
26 tasks
lpinca
lpinca approved these changes Mar 27, 2022
View reviewed changes
Copy link
Member

@lpinca lpinca left a comment

Choose a reason for hiding this comment

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

LGTM with or without the suggested nits fixed.

test/v8-updates/test-trace-gc-flag.js Outdated Show resolved Hide resolved
test/v8-updates/test-trace-gc-flag.js Outdated Show resolved Hide resolved
@github-actions github-actions bot mentioned this pull request Mar 28, 2022
Open
24 tasks
@RaisinTen RaisinTen added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 28, 2022
@nodejs-github-bot
Copy link
Collaborator

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

tony-go and others added 2 commits March 28, 2022 08:55
@tony-go @lpinca
doc: nitpick fix
85d8e3b 
Co-authored-by: Luigi Pinca <luigipinca@gmail.com>
@tony-go
test: use assert.match instead of assert.ok
7442e90
targos
targos reviewed Mar 28, 2022
View reviewed changes
test/v8-updates/test-trace-gc-flag.js Outdated
Comment on lines 30 to 31
const expectedRegex = new RegExp(expectedOutput[index]);
assert.match(line, expectedRegex);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const expectedRegex = new RegExp(expectedOutput[index]);
assert.match(line, expectedRegex);
assert.match(line, expectedOutput[index]);

expectedOutput already contains regexes.

Copy link
Member Author

Choose a reason for hiding this comment

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

In fact, I did like it first, then I got an error but I misread it.

I finally trim the output string and it worked ^^

Thanks a lot 🙏

@tony-go tony-go requested a review from targos March 28, 2022 10:26
@Flarna Flarna added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 28, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 28, 2022
@nodejs-github-bot
Copy link
Collaborator

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

@Flarna Flarna 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. labels Mar 28, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 28, 2022
@nodejs-github-bot nodejs-github-bot merged commit 00cb091 into nodejs:master Mar 28, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in 00cb091

This was referenced Mar 29, 2022
Open
Open
Open
Open
Open
Open
juanarbol pushed a commit that referenced this pull request Apr 4, 2022
@tony-go @juanarbol
test: add trace-gc flag test
22ed08a 
PR-URL: #42471
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
juanarbol pushed a commit to juanarbol/node that referenced this pull request Apr 5, 2022
@tony-go @juanarbol
test: add trace-gc flag test
b756ed8 
PR-URL: nodejs#42471
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
This was referenced Apr 5, 2022
Closed
Merged
@tony-go tony-go mentioned this pull request Apr 6, 2022
Closed
juanarbol pushed a commit that referenced this pull request Apr 6, 2022
@tony-go @juanarbol
test: add trace-gc flag test
52d1c8d 
PR-URL: #42471
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
xtx1130 pushed a commit to xtx1130/node that referenced this pull request Apr 25, 2022
@tony-go @xtx1130
test: add trace-gc flag test
77e0202 
PR-URL: nodejs#42471
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
@tony-go tony-go mentioned this pull request May 5, 2022
Closed
juanarbol pushed a commit that referenced this pull request May 31, 2022
@tony-go @juanarbol
test: add trace-gc flag test
90ecc9b 
PR-URL: #42471
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
@juanarbol juanarbol mentioned this pull request May 31, 2022
Closed
danielleadams pushed a commit that referenced this pull request Jun 27, 2022
@tony-go @danielleadams
test: add trace-gc flag test
91d665f 
PR-URL: #42471
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
targos pushed a commit that referenced this pull request Jul 11, 2022
@tony-go @targos
test: add trace-gc flag test
d9e4998 
PR-URL: #42471
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
targos pushed a commit that referenced this pull request Jul 11, 2022
@tony-go @targos
test: add trace-gc flag test
654bd29 
PR-URL: #42471
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
targos pushed a commit that referenced this pull request Jul 31, 2022
@tony-go @targos
test: add trace-gc flag test
bc60c5d 
PR-URL: #42471
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
@targos targos mentioned this pull request Aug 3, 2022
Merged
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
@tony-go @guangwong
test: add trace-gc flag test
c8447ee 
PR-URL: nodejs/node#42471
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell approved these changes

@lpinca lpinca lpinca approved these changes

@gireeshpunathil gireeshpunathil gireeshpunathil approved these changes

@Flarna Flarna Flarna approved these changes

@RafaelGSS RafaelGSS RafaelGSS approved these changes

@RaisinTen RaisinTen RaisinTen approved these changes

@targos targos Awaiting requested review from targos

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@tony-go @nodejs-github-bot @jasnell @lpinca @targos @gireeshpunathil @Flarna @RafaelGSS @RaisinTen