search

JuliaTime / TimeZones.jl

IANA time zone database access for the Julia programming language
Other
87 stars 53 forks source link

Make Julia nightly tests optional to pass #418

Closed omus closed 1 year ago

omus commented 1 year ago

Based upon this suggestion: https://github.com/actions/runner/issues/2347

codecov-commenter commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

Going to proceed with this.