FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

module: fixup lint regression by guybedford · Pull Request #30802 · nodejs/node · GitHub

/ node Public

module: fixup lint regression - #30802

Closed
guybedford wants to merge 8 commits into
nodejs:masterfrom
guybedford:fixup-lint-failure
ConversationCommits (8)ChecksFiles changed
Closed

module: fixup lint regression#30802
guybedford wants to merge 8 commits into
nodejs:masterfrom
guybedford:fixup-lint-failure

Conversation

guybedford commented Dec 5, 2019

Copy link
Copy Markdown
Contributor

The CI for #30336 was run before the linting change to ensure the use of primordials causing the commit at 1549c8e to fail.

This should resolve that issue.

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

guybedford commented Dec 5, 2019

Copy link
Copy Markdown
Contributor Author

It would be good to have this fast-tracked!

guybedford added the fast-track PRs that do not need to wait for 48 hours to land. label Dec 5, 2019

This comment has been minimized.

Sign in to view

guybedford commented Dec 5, 2019

Copy link
Copy Markdown
Contributor Author

CI is just about there on this one, apart from the flakes of course.

We still need another approval to merge though.

Trott commented Dec 5, 2019

Copy link
Copy Markdown
Member

CI is just about there on this one, apart from the flakes of course.

Not seeing any flakes, but I'm seeing a Windows compilation failure that will prevent the Windows tests from running, so we'll definitely want to do a Resume Build once the rest of the CI finishes.

We still need another approval to merge though.

Yup. /ping @nodejs/modules-active-members

guybedford commented Dec 5, 2019

Copy link
Copy Markdown
Contributor Author

@Trott that Windows failure has been coming up on every CI the past couple of days as far as I can tell.

Trott commented Dec 5, 2019

Copy link
Copy Markdown
Member

@Trott that Windows failure has been coming up on every CI the past couple of days as far as I can tell.

Didn't show up in node-daily-master last night or the night before. We certainly should be landing no code changes whatsoever if Windows CI isn't running.

guybedford commented Dec 5, 2019

Copy link
Copy Markdown
Contributor Author

I believe it is only the windows 2019 build that has been failing.

Trott commented Dec 5, 2019

Copy link
Copy Markdown
Member

I believe it is only the windows 2019 build that has been failing.

If it fails, then the next job (that actually runs the tests!) doesn't run. 😱

Fortunately, it looks like it's been succeeding a lot more than failing: https://ci.nodejs.org/job/node-compile-windows-debug/

The last machine in the CI run (debian9-docker-armv7) is finishing up now so we should be able to try a Resume Build pretty quickly and see what happens.

guybedford commented Dec 5, 2019

Copy link
Copy Markdown
Contributor Author

Yes it's really hard with this many flakes to tell what is a genuine failure. I believe I may have also missed the failure at https://ci.nodejs.org/job/node-test-binary-windows-js-suites/29/ on that merge... I'm looking into this now as well, to see if I can include the fix in this PR.

Trott commented Dec 5, 2019

Copy link
Copy Markdown
Member

Yes it's really hard with this many flakes to tell what is a genuine failure.

Fair.

joaocgreis commented Dec 5, 2019

Copy link
Copy Markdown
Member

Windows CI rebuild: https://ci.nodejs.org/job/node-test-commit-windows-fanned/32010/ - It looks like es-module/test-cjs-esm-warn is a real failure, but I just saw it failing on master as well so I don't know if it's related to this PR.

There were a lot of failures compiling in Windows today, I took a few machines offline and I believe it's mitigated for now. I haven't figured out what's happening yet, but it looks like a missing build dependency.

guybedford commented Dec 5, 2019

Copy link
Copy Markdown
Contributor Author

I'm on the Windows rebuilds here too, no worries! Will cancel and run again due to a lint issue.

guybedford commented Dec 5, 2019

Copy link
Copy Markdown
Contributor Author

(the two latest commits should fix the Windows regression that slipped through into master as well, as mentioned in #30802 (comment))

joaocgreis approved these changes Dec 5, 2019
View reviewed changes

joaocgreis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

test-cjs-esm-warn fix LGTM

Trott reviewed Dec 5, 2019
View reviewed changes
Comment thread test/es-module/test-cjs-esm-warn.js Outdated

Trott commented Dec 5, 2019

Copy link
Copy Markdown
Member

@nodejs/collaborators This could use a second review an 👍 to fast-tracking so we can unbreak CI. PTAL if you're around right now?

Trott commented Dec 5, 2019

Copy link
Copy Markdown
Member

@nodejs/collaborators This could use a second review an 👍 to fast-tracking so we can unbreak CI. PTAL if you're around right now?

Whoops, never mind, I'm three minutes out of date, this has all the needed approvals and can land once we get a green/yellow CI.

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

guybedford commented Dec 5, 2019

Copy link
Copy Markdown
Contributor Author

I tried to retrigger the build on the last suggestion, but can't tell if it's running against the right commit?

Will stop pushing buttons now to avoid further interference, but hopefully we nearly there now.

guybedford commented Dec 5, 2019

Copy link
Copy Markdown
Contributor Author

Ok, CI run seems like it's doing the right thing. Will keep an eye on it here.

guybedford commented Dec 5, 2019

Copy link
Copy Markdown
Contributor Author

Hit another windows failure on the other PR that landed today... that seems itself to be a new flake...!

Is there a way we can just run the Windows CI build?

Trott reviewed Dec 5, 2019
View reviewed changes
Comment thread test/es-module/test-esm-specifiers-both-flags.mjs Outdated

nodejs-github-bot commented Dec 5, 2019

Copy link
Copy Markdown
Collaborator

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

joaocgreis commented Dec 5, 2019

Copy link
Copy Markdown
Member

@guybedford you can go to https://ci.nodejs.org/job/node-test-commit-windows-fanned/ and click "Build with Parameters".

mcollina approved these changes Dec 5, 2019
View reviewed changes

mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

lgtm, +1 to fast-track

guybedford commented Dec 5, 2019

Copy link
Copy Markdown
Contributor Author

Seems there was a Windows issue brought in on #30678 which the timeout was possibly masking. We should either revert that PR or add a fix. I'm running a local windows build now, but don't have much more time here right now.

Trott commented Dec 5, 2019

Copy link
Copy Markdown
Member

Seems there was a Windows issue brought in on #30678 which the timeout was possibly masking. We should either revert that PR or add a fix. I'm running a local windows build now, but don't have much more time here right now.

We have a successful Windows CI on the most recent version of this PR so I'd be inclined to land and address this other issue subsequently if it recurs. Just waiting for the docker build to start and finish at this point....

Trott added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. test Issues and PRs related to the tests. labels Dec 5, 2019

guybedford commented Dec 5, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

These GitHub summaries being out of sync always fool me. Yes it seems we are good.

Trott added the esm Issues and PRs related to the ECMAScript Modules implementation. label Dec 5, 2019

guybedford commented Dec 5, 2019

Copy link
Copy Markdown
Contributor Author

Landed in 781e41d.

guybedford closed this Dec 5, 2019
guybedford added a commit that referenced this pull request Dec 5, 2019
module: fixup lint and test regressions
781e41d 
PR-URL: #30802
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: João Reis <reis@janeasystems.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
targos pushed a commit that referenced this pull request Dec 9, 2019
module: fixup lint and test regressions
a7ec78f 
PR-URL: #30802
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: João Reis <reis@janeasystems.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
MylesBorins mentioned this pull request Dec 13, 2019
Merged
MylesBorins pushed a commit that referenced this pull request Jan 12, 2020
module: fixup lint and test regressions
2a2ae8e 
PR-URL: #30802
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: João Reis <reis@janeasystems.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
targos mentioned this pull request Jan 15, 2020
Closed
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
module: fixup lint and test regressions
77c69f5 
PR-URL: #30802
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: João Reis <reis@janeasystems.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
MylesBorins mentioned this pull request Feb 8, 2020
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Trott Trott approved these changes
mcollina mcollina approved these changes
+1 more reviewer
joaocgreis joaocgreis approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. esm Issues and PRs related to the ECMAScript Modules implementation. fast-track PRs that do not need to wait for 48 hours to land. 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

Back | FazBrowse Home | New Git URL