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

Make Julia nightly tests optional to pass #418

Merged
merged 2 commits into from
Nov 30, 2022
Merged

Make Julia nightly tests optional to pass #418

merged 2 commits into from
Nov 30, 2022

Conversation

omus
Copy link
Member

@omus omus commented Nov 25, 2022

Based upon this suggestion: actions/runner#2347

@codecov-commenter
Copy link

codecov-commenter commented Nov 25, 2022

Codecov Report

Merging #418 (07abaed) into master (daa5f92) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     actions/toolkit#418   +/-   ##
=======================================
  Coverage   95.45%   95.45%           
=======================================
  Files          36       36           
  Lines        1760     1760           
=======================================
  Hits         1680     1680           
  Misses         80       80           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@DilumAluthge
Copy link
Contributor

I'm personally not a fan of using continue-on-error here. If the nightly tests fail, it will show up as a green checkmark, which may confuse users into thinking that the nightly tests are actually passing.

I'd rather we figure out why tests are failing on nightly. If it's always a segfault, then one theory is that multithreading is to blame. So the workaround in that case would be to disable threads when running the nightly tests.

@omus
Copy link
Member Author

omus commented Nov 26, 2022

Looks like this is functioning. Although there are jobs in the workflow which have failed the workflow still is passing (only nightly jobs failed). You can see this by viewing the workflow summary; in this case: https://github.com/JuliaTime/TimeZones.jl/actions/runs/3550945355

Also, viewing the badge for this branch you can see a pass:
https://github.com/JuliaTime/TimeZones.jl/actions/workflows/CI.yml/badge.svg?branch=cv/optional-nightly (once this PR's branch is deleted it'll change to "no status"). Oddly though the indicator on the commit shows a failure. Not a big deal but slightly inconsistent.

@omus
Copy link
Member Author

omus commented Nov 26, 2022

I'm personally not a fan of using continue-on-error here. If the nightly tests fail, it will show up as a green checkmark, which may confuse users into thinking that the nightly tests are actually passing.

As you can see from this PR when the nightly jobs fail they still show up as failures in the checks below. The only real change continue-on-error is doing here is affecting the badge status and the overall workflow status on the "Actions" page.

In the past there have been discussions about running nightly CI tests at all as sometimes the failures there are due to issues introduced into the Julia build rather than from this package itself. Typically when this happens people advocate for removing the nightly tests entirely here. Personally, I like having the tests run against nightly to serve as an early warning system but have them optional to pass.

I'd rather we figure out why tests are failing on nightly. If it's always a segfault, then one theory is that multithreading is to blame. So the workaround in that case would be to disable threads when running the nightly tests.

I dislike the idea of disabling the multithreading tests on nightly as at some point we'd want to re-enable them resulting in unnecessary CI configuration changes. Having them be optional seems like a better approach.

I'll call out that I'm planning on revising how our caching system works with threading in actions/toolkit#382 which will result in our current thead safety test suite in being rewritten our dropped entirely.

@omus omus marked this pull request as ready for review November 26, 2022 22:44
@omus
Copy link
Member Author

omus commented Nov 30, 2022

Going to proceed with this.

@omus omus merged commit fa80e44 into master Nov 30, 2022
@omus omus deleted the cv/optional-nightly branch November 30, 2022 05:15
kpamnany pushed a commit to RelationalAI-oss/TimeZones.jl that referenced this pull request May 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants