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

module: use validateString in modules/esm #24868

Closed
wants to merge 1 commit into from
Closed

module: use validateString in modules/esm #24868

wants to merge 1 commit into from

Conversation

ZYSzys
Copy link
Member

@ZYSzys ZYSzys commented Dec 6, 2018

Almost same as #24863 :)

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@trivikr
Copy link
Member

trivikr commented Dec 8, 2018

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

@trivikr trivikr added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 8, 2018
@Trott
Copy link
Member

Trott commented Dec 8, 2018

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

@Trott
Copy link
Member

Trott commented Dec 9, 2018

Resume Build CI: https://ci.nodejs.org/job/node-test-pull-request/19343/

@ZYSzys
Copy link
Member Author

ZYSzys commented Dec 9, 2018

@Trott It seems to be failed again.😅 Wondering what can I do for it ?

@Trott
Copy link
Member

Trott commented Dec 9, 2018

Yeah, turns out failures are likely relevant. Bug in this PR perhaps?

@Trott Trott removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 9, 2018
@BridgeAR
Copy link
Member

BridgeAR commented Dec 10, 2018 •
edited by Trott
Loading

@Trott the failure looks unrelated to me. The failures from the CI runs also differ, so it's another indicator that it's unrelated. The code change seems to be fine and the functionality should also be identical to the one before.

Resumed CI https://ci.nodejs.org/job/node-test-commit/24157/ ✔️

@Trott
Copy link
Member

Trott commented Dec 10, 2018

@Trott the failure looks unrelated to me. The failures from the CI runs also differ, so it's another indicator that it's unrelated.

Failure on ARM w/gcc 6 was identical in both runs and appears to be related to module loading, so seems likely related to this PR. But yes, I could be wrong.

Refs: https://ci.nodejs.org/job/node-test-commit-arm/20651/
Refs: https://ci.nodejs.org/job/node-test-commit-arm/20647/

@Trott
Copy link
Member

Trott commented Dec 10, 2018 •
edited
Loading

Resume Build was successful, so I guess we're good. Although just to be sure...

ARM CI: https://ci.nodejs.org/job/node-test-commit-arm/20687/ 😞
ARM CI: https://ci.nodejs.org/job/node-test-commit-arm/20688/ ✔️
ARM CI: https://ci.nodejs.org/job/node-test-commit-arm/20689/ 😞

(I expect those will pass, but given the current state of CI, I'm also keen to take extra care that we don't introduce another unreliable test. test-cli-syntax has been brutal....)

Argh, looks like this test is already flaky...

@Trott Trott added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Dec 10, 2018
@Trott
Copy link
Member

Trott commented Dec 10, 2018

Maybe it's already flaky? Let's try the same thing, but on master branch:

ARM CI: https://ci.nodejs.org/job/node-test-commit-arm/20690/
ARM CI: https://ci.nodejs.org/job/node-test-commit-arm/20691/
ARM CI: https://ci.nodejs.org/job/node-test-commit-arm/20692/

@Trott
Copy link
Member

Trott commented Dec 10, 2018

Oh, yeah, already flaky. Here it is failing for #24924: https://ci.nodejs.org/job/node-test-commit-arm/20670/

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 10, 2018
Trott pushed a commit to Trott/io.js that referenced this pull request Dec 10, 2018
@ZYSzys @Trott
module: use validateString in modules/esm
d695a01 
PR-URL: nodejs#24868
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
@Trott
Copy link
Member

Trott commented Dec 10, 2018

Landed in d695a01.

(Sorry for causing an unnecessary delay on landing this.)

@Trott Trott closed this Dec 10, 2018
@ZYSzys ZYSzys deleted the validator-string-esm branch December 11, 2018 08:48
BethGriggs pushed a commit that referenced this pull request Dec 17, 2018
@ZYSzys @BethGriggs
module: use validateString in modules/esm
25dae6c 
PR-URL: #24868
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
@BethGriggs BethGriggs mentioned this pull request Dec 18, 2018
Merged
@beevelop beevelop mentioned this pull request Dec 18, 2018
Closed
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
@ZYSzys @refack
module: use validateString in modules/esm
512d3fb 
PR-URL: nodejs#24868
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
BethGriggs pushed a commit that referenced this pull request Feb 12, 2019
@ZYSzys @BethGriggs
module: use validateString in modules/esm
f2287c6 
PR-URL: #24868
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
@BethGriggs BethGriggs mentioned this pull request Feb 12, 2019
Merged
BethGriggs pushed a commit that referenced this pull request Feb 20, 2019
@ZYSzys @BethGriggs
module: use validateString in modules/esm
2605b6c 
PR-URL: #24868
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
rvagg pushed a commit that referenced this pull request Feb 28, 2019
@ZYSzys @rvagg
module: use validateString in modules/esm
f59c435 
PR-URL: #24868
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@trivikr trivikr trivikr approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@lpinca lpinca lpinca approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@ZYSzys @trivikr @Trott @BridgeAR @lpinca @BethGriggs