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: esm loader unknown builtin module #24183

Closed
wants to merge 1 commit into from
Closed

test: esm loader unknown builtin module #24183

wants to merge 1 commit into from

Conversation

franher
Copy link
Contributor

@franher franher commented Nov 6, 2018

Description

New test added as part of code-learn session. It should not land until #24175 is resolved.

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

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Nov 6, 2018
@mhdawson
Copy link
Member

mhdawson commented Nov 6, 2018

The test looks good to me, and we checked with @BridgeAR who believes it is a problem that he has seen and believes it is a bug in Node.js

@mhdawson mhdawson added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Nov 6, 2018
@franher
Copy link
Contributor Author

franher commented Nov 6, 2018

Tests are now passing after fixing the resolve hook following @devsnek advice (#24175 (comment)).

Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the awesome work!

@franher
Copy link
Contributor Author

franher commented Nov 8, 2018

@BridgeAR hello, do you know why the CI is failing at the linter phase?

It is not indicating a linter error but a CI crash, imho.

Thank you for your support.

@devsnek
Copy link
Member

devsnek commented Nov 8, 2018

seems like the version of node on our ci doesn't support catch without a binding wrt

} catch {
cc @nodejs/build

@richardlau
Copy link
Member

seems like the version of node on our ci doesn't support catch without a binding wrt

node/.eslintrc.js

Line 20 in 1f6c4ba
} catch {
cc @nodejs/build

This was fixed in #24198. I've restarted the Travis CI for this PR.

@franher
Copy link
Contributor Author

franher commented Nov 12, 2018

Looking forward to see this PR landed :)

@gireeshpunathil
Copy link
Member

gireeshpunathil commented Nov 12, 2018

@franher - sure, let me run a full CI and then we should be good to go.
CI: https://ci.nodejs.org/job/node-test-pull-request/18555/

@gireeshpunathil
Copy link
Member

landed as 0229e37 , thanks!

gireeshpunathil pushed a commit that referenced this pull request Nov 12, 2018
PR-URL: #24183
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
BridgeAR pushed a commit that referenced this pull request Nov 14, 2018
PR-URL: #24183
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
kiyomizumia pushed a commit to kiyomizumia/node that referenced this pull request Nov 15, 2018
PR-URL: nodejs#24183
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
codebytere pushed a commit that referenced this pull request Jan 12, 2019
PR-URL: #24183
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@codebytere codebytere mentioned this pull request Jan 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants