Skip to content

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