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

Add persistent_tasks test #174

Merged
merged 15 commits into from Oct 24, 2023
Merged

Add persistent_tasks test #174

merged 15 commits into from Oct 24, 2023

Conversation

timholy
Copy link
Contributor

@timholy timholy commented Sep 10, 2023

This checks whether a package spawns persistent Tasks that might block precompilation of dependents of the package. It also provides a convenience utility, test_persistent_tasks_deps, for checking all the dependencies of a given package, to simplify discovery. If one runs one of the tests in this PR without annotating one as an expected failure, here is the output on Julia 1.10-beta:

julia> Aqua.test_persistent_tasks_deps(getid("UsesBoth"))
  Generating  project jl_jqh8hMTOr6:
    /tmp/jl_jqh8hMTOr6/Project.toml
    /tmp/jl_jqh8hMTOr6/src/jl_jqh8hMTOr6.jl
  Activating project at `/tmp/jl_jqh8hMTOr6`
   Resolving package versions...
    Updating `/tmp/jl_jqh8hMTOr6/Project.toml`
  [94ae9332] + TransientTask v0.1.0 `~/.julia/dev/Aqua/test/pkgs/PersistentTasks/TransientTask`
    Updating `/tmp/jl_jqh8hMTOr6/Manifest.toml`
  [94ae9332] + TransientTask v0.1.0 `~/.julia/dev/Aqua/test/pkgs/PersistentTasks/TransientTask`
  Generating  project jl_FI7OrSlgVA:
    /tmp/jl_FI7OrSlgVA/Project.toml
    /tmp/jl_FI7OrSlgVA/src/jl_FI7OrSlgVA.jl
  Activating project at `/tmp/jl_FI7OrSlgVA`
   Resolving package versions...
    Updating `/tmp/jl_FI7OrSlgVA/Project.toml`
  [e5c298b6] + PersistentTask v0.1.0 `~/.julia/dev/Aqua/test/pkgs/PersistentTasks/PersistentTask`
    Updating `/tmp/jl_FI7OrSlgVA/Manifest.toml`
  [e5c298b6] + PersistentTask v0.1.0 `~/.julia/dev/Aqua/test/pkgs/PersistentTasks/PersistentTask`
PersistentTask [e5c298b6-d81d-47aa-a9ed-5ea983e22986] persistent_tasks: Test Failed at /home/tim/.julia/dev/Aqua/src/persistent_tasks.jl:77
  Expression: fails  precompile_wrapper(result, tmax)

Stacktrace:
 [1] macro expansion
   @ Aqua ~/.julia/juliaup/julia-1.10.0-beta2+0.x64.linux.gnu/share/julia/stdlib/v1.10/Test/src/Test.jl:672 [inlined]
 [2] macro expansion
   @ Aqua ~/.julia/dev/Aqua/src/persistent_tasks.jl:77 [inlined]
 [3] macro expansion
   @ Aqua ~/.julia/juliaup/julia-1.10.0-beta2+0.x64.linux.gnu/share/julia/stdlib/v1.10/Test/src/Test.jl:1577 [inlined]
 [4] test_persistent_tasks(package::PkgId; tmax::Int64, fails::Bool)
   @ Aqua ~/.julia/dev/Aqua/src/persistent_tasks.jl:75
Test Summary:                                                             | Pass  Fail  Total   Time
/home/tim/.julia/dev/Aqua/test/pkgs/PersistentTasks/UsesBoth/Project.toml |    1     1      2  10.2s
  TransientTask [94ae9332-58b0-4342-989c-0a7e44abcca9] persistent_tasks   |    1            1   2.5s
  PersistentTask [e5c298b6-d81d-47aa-a9ed-5ea983e22986] persistent_tasks  |          1      1   7.7s
ERROR: Some tests did not pass: 1 passed, 1 failed, 0 errored, 0 broken.

Closes JuliaLang/julia#50914

CC @vtjnash

@codecov
Copy link

codecov bot commented Sep 10, 2023

Codecov Report

Merging #174 (3f15ae5) into master (69a7655) will decrease coverage by 0.52%.
Report is 1 commits behind head on master.
The diff coverage is 86.20%.

❗ Current head 3f15ae5 differs from pull request most recent head 77f7fd7. Consider uploading reports for the commit 77f7fd7 to get more accurate results

@@            Coverage Diff             @@
##           master     #174      +/-   ##
==========================================
- Coverage   89.63%   89.12%   -0.52%     
==========================================
  Files          10       11       +1     
  Lines         415      570     +155     
==========================================
+ Hits          372      508     +136     
- Misses         43       62      +19     
Flag Coverage Δ
unittests 89.12% <86.20%> (-0.52%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
src/Aqua.jl 72.41% <66.66%> (-27.59%) ⬇️
src/persistent_tasks.jl 87.27% <87.27%> (ø)

... and 9 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Collaborator

@lgoettgens lgoettgens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, this looks like a good improvement for Aqua. Thanks! Unfortunately, the tests did not succeed and the reviewdog has some annotations. I have added some further remarks below.

src/persistent_tasks.jl Outdated Show resolved Hide resolved
src/Aqua.jl Show resolved Hide resolved
src/persistent_tasks.jl Outdated Show resolved Hide resolved
src/persistent_tasks.jl Outdated Show resolved Hide resolved
test/pkgs/PersistentTasks/PersistentTask/Project.toml Outdated Show resolved Hide resolved
test/pkgs/PersistentTasks/TransientTask/Project.toml Outdated Show resolved Hide resolved
test/pkgs/PersistentTasks/UsesBoth/Project.toml Outdated Show resolved Hide resolved
test/test_persistent_tasks.jl Outdated Show resolved Hide resolved
@fingolfin
Copy link
Collaborator

Thank @timholy . Unfortunately CI tests fail in Julia 1.4-1.10 due to a CI timeout -- it seems the tests get suck somehow

src/persistent_tasks.jl Outdated Show resolved Hide resolved
@timholy
Copy link
Contributor Author

timholy commented Sep 11, 2023

Now all passing except Windows. I will investigate later, other duties call.

@timholy
Copy link
Contributor Author

timholy commented Sep 11, 2023

Yay, green.

Copy link
Collaborator

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much for this wonderful contribution.

I have one remaining concern: there is an established pattern for signalling a broken test case, using a broken kwarg; it would be nice if this PR also used it, instead of using fails for essentially the same purpose. Would you mind doing that? If not, I'll try to make the necessary adjustments but it may take me a while to find the time (sorry)

src/persistent_tasks.jl Outdated Show resolved Hide resolved
src/persistent_tasks.jl Outdated Show resolved Hide resolved
@timholy
Copy link
Contributor Author

timholy commented Sep 21, 2023

I changed the name to broken but I'm no longer sure that was the right thing to do. Is broken intended to used by packages that are using Aqua for their tests? I.e., "gosh, I want this Aqua test to pass, but it doesn't, some day I should fix my package." In contrast, fails was purely for veracity of Aqua's own internal testing; it's not intended to be used by "consumers" of Aqua.

@timholy
Copy link
Contributor Author

timholy commented Sep 21, 2023

I see a "needs changelog" label but I don't see a "NEWS" file?

@lgoettgens
Copy link
Collaborator

I see a "needs changelog" label but I don't see a "NEWS" file?

It has only been introduced this week. If you rebase your branch onto latest master, you should find a CHANGELOG.md.

@lgoettgens
Copy link
Collaborator

I'll later have a look at your issue with broken and fails

@lgoettgens
Copy link
Collaborator

I have slightly altered the interface. Many of the issues here can easily be solved by splitting test functions up into one computing something and another one checking it with @test.
has_persistent_tasks now returns a boolean. test_persistent_tasks just checks this boolean (depending of a kwarg broken with @test or @test_broken). Internal testing just tests has_persistent_tasks.
Similar to test_persistent_tasks_deps: I changed it to a function returning a vector of the failing packages instead. This is almost as readable as an output but gives users access to the results (if they want to use it somehow), and makes testing this a lot cleaner.

Similar adaptations are happening one after the other to the other, already for longer time existing tasks as well. I hope these changes are fine for you. Please check that I didn't break everything. Thanks!

@lgoettgens
Copy link
Collaborator

lgoettgens commented Sep 21, 2023

I noticed some caveats/errors with this new test. Aqua tests itself, and when enabling test_persistent_tasks, that task fails. That may possibly be some bug in the persisten_tasks code.

Furthermore, find_persistent_tasks_deps fails if a dependency is in the sysimg. For example, try to call it with Aqua as its argument.

It would be great if you could look into these two things and fix them.

src/persistent_tasks.jl Outdated Show resolved Hide resolved
@lgoettgens
Copy link
Collaborator

Rebased this on top of the 0.7.3 release

Copy link
Collaborator

@lgoettgens lgoettgens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found the culprits (see comments below). Furthermore I already pushed a quality of life change, that restores the active environment after the test.

@timholy please have a look, if this looks good to you. If the suggestions get committed, this is good to go from my pov.

src/persistent_tasks.jl Show resolved Hide resolved
src/persistent_tasks.jl Show resolved Hide resolved
src/persistent_tasks.jl Outdated Show resolved Hide resolved
@timholy
Copy link
Contributor Author

timholy commented Oct 23, 2023

Sorry for my long absence here, and many thanks for carrying this forward. Just to check (since there have been many commits), are the comments in #174 (review) still relevant? They seem like nice changes to me. If you want I can update this, or feel free to do so yourself. Aside from some travel later this week, I am getting back to a point of having time to finish this off.

@lgoettgens
Copy link
Collaborator

Sorry for my long absence here, and many thanks for carrying this forward. Just to check (since there have been many commits), are the comments in #174 (review) still relevant? They seem like nice changes to me. If you want I can update this, or feel free to do so yourself. Aside from some travel later this week, I am getting back to a point of having time to finish this off.

Yes, these comments/suggestions are still relevant. I just rebase this onto master to resolve some conflicts. I think accepting all suggestions should resolve the remaining ci failures

@lgoettgens
Copy link
Collaborator

You don't need to worry about the failures in julia 1.3 and before, as we will drop support anyway (see #216).

@lgoettgens lgoettgens modified the milestones: 0.7.x, 0.8 Oct 24, 2023
timholy and others added 14 commits October 24, 2023 16:14
Co-authored-by: "Lars Göttgens <lars.goettgens@rwth-aachen.de>"
Flushing the io may prevent some of the CI hangs;
also print diagnostics in case of unexpected outcomes.
Co-authored-by: Max Horn <max@quendi.de>
Co-authored-by: Max Horn <max@quendi.de>
Co-authored-by: Lars Göttgens <lars.goettgens@gmail.com>
@lgoettgens lgoettgens force-pushed the teh/exit branch 2 times, most recently from 3f15ae5 to 77f7fd7 Compare October 24, 2023 14:26
@lgoettgens lgoettgens merged commit b2d612c into JuliaTesting:master Oct 24, 2023
18 checks passed
@timholy timholy deleted the teh/exit branch October 30, 2023 16:48
@timholy
Copy link
Contributor Author

timholy commented Oct 30, 2023

Thanks for all the help, @lgoettgens, and others for their reviews!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants