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: fixup lint regression #30802

Closed
wants to merge 8 commits into from
Closed

module: fixup lint regression #30802

wants to merge 8 commits into from

Conversation

guybedford
Copy link
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
Copy link
Contributor Author

It would be good to have this fast-tracked!

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

This comment has been minimized.

Sign in to view
@guybedford
Copy link
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
Copy link
Member

Trott commented Dec 5, 2019

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
Copy link
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
Copy link
Member

Trott commented Dec 5, 2019

@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
Copy link
Contributor Author

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

@Trott
Copy link
Member

Trott commented Dec 5, 2019

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
Copy link
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
Copy link
Member

Trott commented Dec 5, 2019

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

Fair.

@joaocgreis
Copy link
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
Copy link
Contributor Author

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

@guybedford
Copy link
Contributor Author

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

joaocgreis
joaocgreis approved these changes Dec 5, 2019
View reviewed changes
Copy link
Member

@joaocgreis joaocgreis left a comment

Choose a reason for hiding this comment

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

test-cjs-esm-warn fix LGTM

@Trott
Copy link
Member

Trott commented Dec 5, 2019

@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
Copy link
Member

Trott commented Dec 5, 2019

@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.

@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@guybedford
Copy link
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
Copy link
Contributor Author

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

@guybedford
Copy link
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?

@nodejs-github-bot
Copy link
Collaborator

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

@joaocgreis
Copy link
Member

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

mcollina
mcollina approved these changes Dec 5, 2019
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm, +1 to fast-track

@guybedford
Copy link
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
Copy link
Member

Trott commented Dec 5, 2019

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 Trott added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. test Issues and PRs related to the tests. labels Dec 5, 2019
@guybedford
Copy link
Contributor Author

guybedford commented Dec 5, 2019 •
edited
Loading

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

@Trott Trott added the esm Issues and PRs related to the ECMAScript Modules implementation. label Dec 5, 2019
@guybedford
Copy link
Contributor Author

Landed in 781e41d.

@guybedford guybedford closed this Dec 5, 2019
guybedford added a commit that referenced this pull request Dec 5, 2019
@guybedford
module: fixup lint and test regressions
781e41d 
PR-URL: #30802
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: João Reis <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
targos pushed a commit that referenced this pull request Dec 9, 2019
@guybedford @targos
module: fixup lint and test regressions
a7ec78f 
PR-URL: #30802
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: João Reis <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Dec 13, 2019
Merged
MylesBorins pushed a commit that referenced this pull request Jan 12, 2020
@guybedford @MylesBorins
module: fixup lint and test regressions
2a2ae8e 
PR-URL: #30802
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: João Reis <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@targos targos mentioned this pull request Jan 15, 2020
Closed
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
@guybedford @BethGriggs
module: fixup lint and test regressions
77c69f5 
PR-URL: #30802
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: João Reis <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Feb 8, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Trott Trott Trott approved these changes

@joaocgreis joaocgreis joaocgreis approved these changes

@mcollina mcollina mcollina 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. 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
@guybedford @nodejs-github-bot @Trott @joaocgreis @mcollina