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.45% 87.76% +0.31%
==========================================
Files 12 12
Lines 526 523 -3
==========================================
- Hits 460 459 -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
| precompilation process being unable to write its cache and exit: it hangs | ||
| indefinitely. A package without persistent tasks always exits eventually, so | ||
| Aqua waits up to `tmax` seconds for a clean shutdown before reporting a failure. | ||
| With many or slow-to-precompile dependencies this shutdown can be slow, so if a |
There was a problem hiding this comment.
I don't quite follow this. slow-to-precompile dependencies should only increase the time before loading PkgA, but not the shutdown, doesn't it?
There was a problem hiding this comment.
Yes, thanks, that was worded really unfortunately. If you run a separate test process, a large tmax value effectively could result in a long time until the test process is finally shut down (better wording: has finished).
I changed the wording and took the opportunity to shorten the newly introduced comments. I think they have been a bit over the top.
## 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. |
2 participants
tmaxconsistently to 30 seconds to reduce false positives when e.g. the system load is high.Fixes: #315