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: skip experimental test with pointer compression #48738

Merged
merged 2 commits into from
Jul 16, 2023

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Jul 11, 2023

The test test/parallel/test-experimental-shared-value-conveyor.js was added to test the --harmony-struct feature of V8. However, when used with pointer compression, the process crashes. This commit skips the test for pointer compression builds. This change uses a child process because starting a Node pointer compression build with --harmony-struct immediately crashes the process. Once this crash is addressed, this commit can be reverted.

Refs: nodejs/build#3204 (comment)

@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 Jul 11, 2023
The test test/parallel/test-experimental-shared-value-conveyor.js was
added to test the --harmony-struct feature of V8. However, when used
with pointer compression, the process crashes. This commit skips
the test for pointer compression builds. This change uses a child
process because starting a Node pointer compression build with
--harmony-struct immediately crashes the process. Once this crash
is addresses, this commit can be reverted.
@MoLow
Copy link
Member

MoLow commented Jul 11, 2023

should the test be part of known_issues?

@cjihrig
Copy link
Contributor Author

cjihrig commented Jul 11, 2023

I would prefer not to use known_issues since it's specific to a single build type with an experimental flag. Putting something in known_issues means making sure the test always fails.

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

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

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

@nodejs-github-bot
Copy link
Collaborator

@lpinca
Copy link
Member

lpinca commented Jul 14, 2023

CI failures seem realted.

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@richardlau richardlau added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Jul 15, 2023
@cjihrig cjihrig added commit-queue Add this label to land a pull request using GitHub Actions. and removed needs-ci PRs that need a full CI run. labels Jul 15, 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 Jul 15, 2023
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/48738
✔  Done loading data for nodejs/node/pull/48738
----------------------------------- PR info ------------------------------------
Title      test: skip experimental test with pointer compression (#48738)
Author     Colin Ihrig  (@cjihrig)
Branch     cjihrig:pc-test -> nodejs:main
Labels     test, author ready, commit-queue-squash
Commits    2
 - test: skip experimental test with pointer compression
 - Update test/parallel/test-experimental-shared-value-conveyor.js
Committers 2
 - cjihrig 
 - GitHub 
PR-URL: https://github.com/nodejs/node/pull/48738
Reviewed-By: Moshe Atlow 
Reviewed-By: Michael Dawson 
Reviewed-By: Matteo Collina 
Reviewed-By: Debadree Chatterjee 
Reviewed-By: Luigi Pinca 
Reviewed-By: Richard Lau 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/48738
Reviewed-By: Moshe Atlow 
Reviewed-By: Michael Dawson 
Reviewed-By: Matteo Collina 
Reviewed-By: Debadree Chatterjee 
Reviewed-By: Luigi Pinca 
Reviewed-By: Richard Lau 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Tue, 11 Jul 2023 16:24:54 GMT
   ✔  Approvals: 6
   ✔  - Moshe Atlow (@MoLow) (TSC): https://github.com/nodejs/node/pull/48738#pullrequestreview-1524698864
   ✔  - Michael Dawson (@mhdawson) (TSC): https://github.com/nodejs/node/pull/48738#pullrequestreview-1525014643
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/48738#pullrequestreview-1525697808
   ✔  - Debadree Chatterjee (@debadree25): https://github.com/nodejs/node/pull/48738#pullrequestreview-1529126202
   ✔  - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/48738#pullrequestreview-1529136855
   ✔  - Richard Lau (@richardlau) (TSC): https://github.com/nodejs/node/pull/48738#pullrequestreview-1531530821
   ✘  Last GitHub CI failed
   ℹ  Last Full PR CI on 2023-07-15T16:50:49Z: https://ci.nodejs.org/job/node-test-pull-request/52768/
- Querying data for job/node-test-pull-request/52768/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/5563587926

@MoLow MoLow added the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 16, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 16, 2023
@nodejs-github-bot nodejs-github-bot merged commit a4e4a86 into nodejs:main Jul 16, 2023
33 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in a4e4a86

@cjihrig cjihrig deleted the pc-test branch July 16, 2023 14:29
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
The test test/parallel/test-experimental-shared-value-conveyor.js was
added to test the --harmony-struct feature of V8. However, when used
with pointer compression, the process crashes. This commit skips
the test for pointer compression builds. This change uses a child
process because starting a Node pointer compression build with
--harmony-struct immediately crashes the process. Once this crash
is addresses, this commit can be reverted.

PR-URL: nodejs#48738
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Debadree Chatterjee <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
The test test/parallel/test-experimental-shared-value-conveyor.js was
added to test the --harmony-struct feature of V8. However, when used
with pointer compression, the process crashes. This commit skips
the test for pointer compression builds. This change uses a child
process because starting a Node pointer compression build with
--harmony-struct immediately crashes the process. Once this crash
is addresses, this commit can be reverted.

PR-URL: nodejs#48738
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Debadree Chatterjee <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
UlisesGascon pushed a commit to UlisesGascon/node that referenced this pull request Aug 14, 2023
The test test/parallel/test-experimental-shared-value-conveyor.js was
added to test the --harmony-struct feature of V8. However, when used
with pointer compression, the process crashes. This commit skips
the test for pointer compression builds. This change uses a child
process because starting a Node pointer compression build with
--harmony-struct immediately crashes the process. Once this crash
is addresses, this commit can be reverted.

PR-URL: nodejs#48738
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Debadree Chatterjee <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
RafaelGSS pushed a commit that referenced this pull request Aug 15, 2023
The test test/parallel/test-experimental-shared-value-conveyor.js was
added to test the --harmony-struct feature of V8. However, when used
with pointer compression, the process crashes. This commit skips
the test for pointer compression builds. This change uses a child
process because starting a Node pointer compression build with
--harmony-struct immediately crashes the process. Once this crash
is addresses, this commit can be reverted.

PR-URL: #48738
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Debadree Chatterjee <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
@UlisesGascon UlisesGascon mentioned this pull request Aug 15, 2023
This pull request was closed.
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-failed An error occurred while landing this 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. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants