Make persistent task test more robust - #389
Conversation
- Reports a precompilation failure as a precompilation error instead of misclassifying it as a persistent task. - Set the default for `tmax` consistently to 30 seconds to reduce false positives when e.g. the system load is high. Fixes: JuliaTesting#315
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #389 +/- ##
==========================================
+ Coverage 87.50% 87.80% +0.30%
==========================================
Files 12 12
Lines 528 525 -3
==========================================
- Hits 462 461 -1
+ Misses 66 64 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This tests the persistent task logic.
|
@lgoettgens: Anything I can do to improve the chances to get this PR merged? |
|
I didn't notice this, sorry. It's now on my todo list |
|
Thanks for having a look when reaching it on your TODO list – much appreciated! |
lgoettgens
left a comment
lgoettgens
left a comment
There was a problem hiding this comment.
There now also is the PR #390 which tries to tackle the same problem. I asked the author of that to explain differences to this PR.
I will eventually try to take the best parts of both of these
## PR JuliaTesting#389 vs PR JuliaTesting#390 **JuliaTesting#389 (`more_robust_persistent_task`, this branch)** — Fixes the root cause: splits the wait into an *unbounded* load phase and a *`tmax`-bounded* shutdown phase, so dependency precompilation no longer counts toward the persistent-task verdict. Raises `tmax` 5→30. Reports a precompile failure by *throwing an error* (with captured stderr), not by mislabeling it a persistent task. **JuliaTesting#390 (`report-precompile-failure-in-persistent-tasks`)** — Only reports *why* the probe failed: captures `Pkg.precompile`'s `io` output to a file and logs an `@error` with `exitcode`/`termsignal`, then returns `true`. Leaves the timing model and `tmax` untouched. **Why we took over only `termsignal`:** JuliaTesting#389 already captures **stderr**, and Julia sends exceptions *and* warnings there (`Pkg.precompile`'s default is `io = stderr`). So JuliaTesting#390's separate `io`-to-file capture is redundant — the same information through a different door. The only genuinely additive detail was `termsignal`, which distinguishes a signal-killed subprocess from a normal nonzero exit.
As Claude had my branch loaded anyway, I let it compare with @ChrisRackauckas branch. Opus told me that #389 were the better base. I let it then take over from #390 what was better than in #389 and that is the most recent commit. It was not too much. The commit is okay and it looked plausible overall, but I did not dive into #390. |
Co-authored-by: Lars Göttgens <lars.goettgens@gmail.com>
Co-authored-by: Lars Göttgens <lars.goettgens@gmail.com>
|
Thanks a lot, @lgoettgens, for the improvements and for merging the improved version! |
2 participants
tmaxconsistently to 30 seconds to reduce false positives when e.g. the system load is high.Fixes #315, Closes #390.