Skip to content

Conversation

@BethGriggs
Copy link
Member

Initially opening as a test to see if this fixes our actions runs.

Refs:


This reverts commit 78343bb.

@nodejs-github-botnodejs-github-bot added needs-ci PRs that need a full CI run. util Issues and PRs related to the built-in util module. labels Apr 14, 2021
@BethGriggs
Copy link
MemberAuthor

Looks like #38211 was dependent on this, so this revert is failing the linter run. I'll see how the other runs go, but I'm guessing we'd potentially need to revert both to get back to passing runs.

@nodejs-github-bot
Copy link
Collaborator

@targos
Copy link
Member

Even if this makes the run green I'm not convinced it's the right move. The reverted PR did not introduce the bug, it made it more visible.

@BethGriggs
Copy link
MemberAuthor

@targos, the motivation for this PR was that when the actions are not green across the board, it raises the chance of other failures slipping through (and it appears PR #37263 landed with the failures).

Do you have an alternative suggestion? I skimmed through the linked issues, and #38000 did not appear to be imminently ready to land.

@targos
Copy link
Member

The alternative is to skip or mark the test flaky

@jasnell
Copy link
Member

If the test is failing consistently on every run then it's not flaky, it's just broken. My preference would be to go ahead and revert so that we can make sure CI is green, fix the test, then re-land the reverted change.

@targos
Copy link
Member

If something should be reverted, it's the change that broke the test in the first place, not another unrelated change that happens to make it red in CI. That test has been failing on my machine for several days already

@aduh95
Copy link
Contributor

Looks like #38211 was dependent on this, so this revert is failing the linter run. I'll see how the other runs go, but I'm guessing we'd potentially need to revert both to get back to passing runs.

Actually it's not dependent, you only need to use globalThis from primordials to fix the linter issue. I can take care of this if you want.

@BethGriggs
Copy link
MemberAuthor

BethGriggs commented Apr 14, 2021

Actually it's not dependent, you only need to use globalThis from primordials to fix the linter issue. I can take care of this if you want.

I meant dependent in the sense that I couldn't just revert 78343bb alone. I'll leave fixing the actions to folks who are more familiar with errors.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-ciPRs that need a full CI run.utilIssues and PRs related to the built-in util module.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@BethGriggs@nodejs-github-bot@targos@jasnell@aduh95