Skip to content

Conversation

@jnord99
Copy link
Contributor

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@nodejs-github-botnodejs-github-bot added the test Issues and PRs related to the tests. label Oct 12, 2018
@BridgeARBridgeAR added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Oct 12, 2018
@gdams
Copy link
Member

FYI I am currently working on adding Mojave to ci.nodejs.org. Should be in there by the end of the day

Copy link
Member

@devsnekdevsnek left a comment

Choose a reason for hiding this comment

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

rip privileged escalation 😢

@Trott
Copy link
Member

One downside is that the test will continue to be skipped after the bug in Mojave is fixed.

@gdams
Copy link
Member

@gdamsgdams added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 15, 2018
@refack
Copy link
Contributor

One downside is that the test will continue to be skipped after the bug in Mojave is fixed.

@jnord99 could you add test that fails to /test/known_issues/ (where we expect test to fail until the underlying issue is resolved)?

@cjihrig
Copy link
Contributor

I would not recommend moving them to known_issues.

@Trott
Copy link
Member

I don't think this is a good candidate for known_issues because you'll need to list all the places where it will succeed in known_issues.status and there's no way to specify macOS Mojave only.

@refack
Copy link
Contributor

I would not recommend moving them to known_issues.

Ack, Not moving. Adding a new one with the opposite condition, i.e.:

// Fail on OS X Mojave. https://github.com/nodejs/node/issues/21679if(!common.isOSXMojave)common.skip('bypass test for non macOS Mojave'); ... Dosomethingthatfails ...

@jnord99
Copy link
ContributorAuthor

the update skips the test if it is Mojave. It already was doing the same for Windows. It will execute the test for other systems.
Yes, right now it is not granular. It currently looks for the base level of the OS and skips based on that major version. The node os module shows the version as 18. the test will skip for every os shown as 'darwin' and starts with 18. It could be revised to only skip if 'darwin' and 18.0.0 as that is what os.release() shows as the current version. If they fix it in 18.0.1 then the test will pass. if not then that would need to add 18.0.1 and so on until fixed.
current thinking was to just put it in as a major level skip and it could be updated if and when Apple corrects the issue. I don't know if that should be added as a known version or how best to track it.

@refack
Copy link
Contributor

refack commented Oct 15, 2018

Added a "known_issue" test in https://github.com/nodejs/node/pull/23550/commits/cbc55b0874bd9742fd33cb27a14b4dd94d0e48d9to track this. Unfortunately we can't CI it.

@nodejs/platform-macos anyone can verify?
(python tools/test.py known_issues)

Copy link
Member

@mhdawsonmhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@ChALkeRChALkeR left a comment

Choose a reason for hiding this comment

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

LGTM.

One downside is that the test will continue to be skipped after the bug in Mojave is fixed.

That is correct, but this is obviously better than scaring off all macOS users from contributing because they can't get tests to pass on their platform.

We can revisit it later once/if the issue gets fixed.

Also, this checks specifically for =18, not >=18.
So any macOS version later than Mojave would have this test running again, either passing (if that gets fixed) or failing (so we revisit again and would have to update the test once more).

@Trott
Copy link
Member

@addaleax
Copy link
Member

Landed in eff869f

Thanks for the contribution! 🎉

(If you're interested in other possible contributions to Node.js but don't have a good idea of where to start looking, some ideas are posted at https://www.nodetodo.org/next-steps/.)

addaleax pushed a commit that referenced this pull request Oct 21, 2018
Refs: #21679 PR-URL: #23550 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: George Adams <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Сковорода Никита Андреевич <[email protected]>
jasnell pushed a commit that referenced this pull request Oct 21, 2018
Refs: #21679 PR-URL: #23550 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: George Adams <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Сковорода Никита Андреевич <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 30, 2018
Refs: #21679 PR-URL: #23550 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: George Adams <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Сковорода Никита Андреевич <[email protected]>
@BethGriggsBethGriggs mentioned this pull request Oct 30, 2018
MylesBorins pushed a commit that referenced this pull request Nov 26, 2018
Refs: #21679 PR-URL: #23550 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: George Adams <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Сковорода Никита Андреевич <[email protected]>
@codebyterecodebytere mentioned this pull request Nov 27, 2018
rvagg pushed a commit that referenced this pull request Nov 28, 2018
Refs: #21679 PR-URL: #23550 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: George Adams <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Сковорода Никита Андреевич <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
Refs: #21679 PR-URL: #23550 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: George Adams <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Сковорода Никита Андреевич <[email protected]>
@codebyterecodebytere mentioned this pull request Nov 29, 2018
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.code-and-learnIssues related to the Code-and-Learn events and PRs submitted during the events.testIssues and PRs related to the tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

15 participants

@jnord99@gdams@Trott@refack@cjihrig@addaleax@ChALkeR@jasnell@santigimeno@devsnek@BridgeAR@mhdawson@trivikr@MylesBorins@nodejs-github-bot