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.

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

test: skip failing tests for osx mojave #23550

Closed
wants to merge 4 commits into from

Conversation

jnord99
Copy link
Contributor

@jnord99 jnord99 commented Oct 12, 2018

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

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 12, 2018
@BridgeAR BridgeAR 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

gdams commented Oct 12, 2018

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

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

Trott commented Oct 12, 2018

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

@gdams
Copy link
Member

gdams commented Oct 15, 2018

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

refack commented Oct 15, 2018

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

cjihrig commented Oct 15, 2018

I would not recommend moving them to known_issues.

@Trott
Copy link
Member

Trott commented Oct 15, 2018

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

refack commented Oct 15, 2018

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/21679
if (!common.isOSXMojave)
  common.skip('bypass test for non macOS Mojave');

... Do something that fails ...

@jnord99
Copy link
Contributor Author

jnord99 commented Oct 15, 2018

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

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

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

Trott commented Oct 18, 2018

@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 addaleax closed this Oct 21, 2018
addaleax pushed a commit that referenced this pull request Oct 21, 2018
Refs: #21679
PR-URL: #23550
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: George Adams <george.adams@uk.ibm.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
jasnell pushed a commit that referenced this pull request Oct 21, 2018
Refs: #21679
PR-URL: #23550
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: George Adams <george.adams@uk.ibm.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
MylesBorins pushed a commit that referenced this pull request Oct 30, 2018
Refs: #21679
PR-URL: #23550
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: George Adams <george.adams@uk.ibm.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
@BethGriggs BethGriggs 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 <ruben@bridgewater.de>
Reviewed-By: George Adams <george.adams@uk.ibm.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
@codebytere codebytere 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 <ruben@bridgewater.de>
Reviewed-By: George Adams <george.adams@uk.ibm.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
Refs: #21679
PR-URL: #23550
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: George Adams <george.adams@uk.ibm.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
@codebytere codebytere mentioned this pull request Nov 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.