GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — module: fixup lint regression by guybedford · Pull Request #30802 · nodejs/node

Conversation

@guybedford

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@guybedford
module: fixup lint regression
6fc1b9b
@guybedford

guybedford commented Dec 5, 2019

Copy link
Copy Markdown
Contributor Author

It would be good to have this fast-tracked!
Trott, joaocgreis, and gengjiawen reacted with thumbs up emoji
All reactions
  • 👍 3 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@guybedford guybedford added the fast-track PRs proposed for a shorter-than-standard waiting period before landing. label Dec 5, 2019
@nodejs-github-bot

This comment has been minimized.

Sign in to view
Trott
Trott approved these changes Dec 5, 2019
View reviewed changes
@guybedford

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Trott

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@guybedford

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Trott

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@guybedford

guybedford commented Dec 5, 2019

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Trott

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@guybedford

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Trott

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@guybedford
fixup windows bug attempt
17f0235
@joaocgreis

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@guybedford
fixup quotes
7e916c6
@guybedford

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.
joaocgreis reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@guybedford

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))
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
joaocgreis
joaocgreis approved these changes Dec 5, 2019
View reviewed changes

@joaocgreis joaocgreis left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Trott
Trott reviewed Dec 5, 2019
View reviewed changes
Comment thread test/es-module/test-cjs-esm-warn.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
@guybedford
fixup regex global
1bb808f
@Trott

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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Trott

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@guybedford
further test refinements
b005b92
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@guybedford
remove unnecessary line
45a835c
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@guybedford

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@guybedford

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@guybedford
try increase timeout
3698b28
@guybedford

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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Trott
Trott reviewed Dec 5, 2019
View reviewed changes
Comment thread test/es-module/test-esm-specifiers-both-flags.mjs Resolved

Uh oh!

There was an error while loading. Please reload this page.
@guybedford
remove timeout
3903f5a
@nodejs-github-bot

nodejs-github-bot commented Dec 5, 2019

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@joaocgreis

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".
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
mcollina
mcollina approved these changes Dec 5, 2019
View reviewed changes

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@guybedford

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Trott

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....
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Trott Trott added author ready PRs with CI started, the required approvals, and no outstanding review comments. test Issues and PRs related to Node.js core tests and test infrastructure. labels Dec 5, 2019
@guybedford

guybedford commented Dec 5, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Trott Trott added the esm Issues and PRs related to the ECMAScript Modules implementation. label Dec 5, 2019
@guybedford

guybedford commented Dec 5, 2019

Copy link
Copy Markdown
Contributor Author

Landed in 781e41d.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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
v13.4.0 proposal #30937
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
v12.15.0 release proposal #31368
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
v12.16.0 proposal #31691
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
@mcollina mcollina mcollina approved these changes
+1 more reviewer
@joaocgreis joaocgreis joaocgreis approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. esm Issues and PRs related to the ECMAScript Modules implementation. fast-track PRs proposed for a shorter-than-standard waiting period before landing. test Issues and PRs related to Node.js core tests and test infrastructure.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@guybedford @nodejs-github-bot @Trott @joaocgreis @mcollina
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.