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: temporarily disable MultipleEnvironmentsPerIsolate #14246

Closed
wants to merge 2 commits into from
Closed

test: temporarily disable MultipleEnvironmentsPerIsolate #14246

wants to merge 2 commits into from

Conversation

refack
Copy link
Contributor

@refack refack commented Jul 15, 2017 •
edited
Loading

Ref: #14206

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

test,cctest

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Jul 15, 2017
@refack
Copy link
Contributor Author

refack commented Jul 15, 2017

/cc @nodejs/testing @mhdawson @jBarz
Let's fast track this band-aid.

Also I've set a calendar reminder to revisit on Wednesday 7/14

@refack refack added flaky-test Issues and PRs related to the tests with unstable failures on the CI. lib / src Issues and PRs related to general changes in the lib or src directory. labels Jul 15, 2017
@Trott
Copy link
Member

Trott commented Jul 15, 2017

I don't suppose there's any already-existing and easy way to disable it in some environments only?

@refack
Copy link
Contributor Author

refack commented Jul 15, 2017

I don't suppose there's any already-existing and easy way to disable it in some environments only?

Maybe some #ifndef voodoo, but I'm not going there 🙌

  1. We have 4 people keeping an eye on this
  2. AFAICT it's the only place we create two Envs so even if we forget this and it rots, it's gonna be a while until we encounter this scenario in real code.

Trott
Trott approved these changes Jul 15, 2017
View reviewed changes
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

LGTM if CI is green.

In principle, I'm on board with fast-tracking this, but I'd want an additional 👍 on that from someone who has a better understanding of our C++ code.

@refack
Copy link
Contributor Author

refack commented Jul 15, 2017

CI: https://ci.nodejs.org/job/node-test-commit/11123/
@Trott you wanted this to land in 24 hours for the code-and-learn, right?

@Trott
Copy link
Member

Trott commented Jul 15, 2017

@Trott you wanted this to land in 24 hours for the code-and-learn, right?

That would be great! If not, though, I can probably remove plinux from the build infra for the duration of Code + Learn, so it's not critical that it happen.

@refack
Copy link
Contributor Author

refack commented Jul 15, 2017

@nodejs/collaborators we would like another opinion on this, thank you...

@gibfahn
Copy link
Member

gibfahn commented Jul 16, 2017 •
edited
Loading

I think as long as #14206 stays open (and we keep investigating it) then doing this is fine.

Unfortunately I don't "have a better understanding of our cpp code". But I don't think there's a problem with doing this.

@gibfahn
Copy link
Member

gibfahn commented Jul 16, 2017

That would be great! If not, though, I can probably remove plinux from the build infra for the duration of Code + Learn, so it's not critical that it happen.

I'd much rather do this than remove a platform from CI.

@Trott
Copy link
Member

Trott commented Jul 16, 2017

Landed in 95ab966

@Trott Trott closed this Jul 16, 2017
Trott pushed a commit that referenced this pull request Jul 16, 2017
@refack @Trott
test: disable MultipleEnvironmentsPerIsolate
95ab966 
This is a temporary measure until the issue is fixed.

PR-URL: #14246
Refs: #14206
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
@addaleax addaleax mentioned this pull request Jul 18, 2017
Merged
addaleax pushed a commit that referenced this pull request Jul 18, 2017
@refack @addaleax
test: disable MultipleEnvironmentsPerIsolate
9efd328 
This is a temporary measure until the issue is fixed.

PR-URL: #14246
Refs: #14206
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Fishrock123 pushed a commit that referenced this pull request Jul 19, 2017
@refack @Fishrock123
test: disable MultipleEnvironmentsPerIsolate
d2e0f70 
This is a temporary measure until the issue is fixed.

PR-URL: #14246
Refs: #14206
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
@refack refack deleted the temp-disable-MultipleEnvironmentsPerIsolate branch July 20, 2017 00:15
addaleax added a commit to addaleax/node that referenced this pull request Aug 10, 2017
@addaleax
Revert "test: disable MultipleEnvironmentsPerIsolate"
557eb7b 
This reverts commit 95ab966.

Ref: nodejs#14206
Ref: nodejs#14246
addaleax added a commit that referenced this pull request Aug 10, 2017
@addaleax
Revert "test: disable MultipleEnvironmentsPerIsolate"
c80d400 
This reverts commit 95ab966.

Ref: #14206
Ref: #14246
PR-URL: #14749
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
addaleax added a commit that referenced this pull request Aug 10, 2017
@addaleax
Revert "test: disable MultipleEnvironmentsPerIsolate"
a781bb4 
This reverts commit 95ab966.

Ref: #14206
Ref: #14246
PR-URL: #14749
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
@MylesBorins
Copy link
Contributor

as we are not having issue with cc tests on v6.x I think we can pass on this for now.

@refack
Copy link
Contributor Author

refack commented Aug 19, 2017 •
edited
Loading

Agreed.
(underlining bug was solved, so this was reverted anyway)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gibfahn gibfahn gibfahn approved these changes

@Trott Trott Trott approved these changes

Assignees
No one assigned
Labels
flaky-test Issues and PRs related to the tests with unstable failures on the CI. lib / src Issues and PRs related to general changes in the lib or src directory. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@refack @Trott @gibfahn @MylesBorins @nodejs-github-bot