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

test_persistent_tasks: Add an optional expr to run in the precompile package #255

Merged
merged 7 commits into from
Dec 1, 2023

Conversation

NHDaly
Copy link
Member

@NHDaly NHDaly commented Nov 29, 2023

(From this slack discussion)

Here's an example of it working!

julia> Aqua.test_persistent_tasks(MuttsDicts, quote
           Threads.@spawn while true sleep(0.5) end
       end)
...

Precompiling project...
        Info Given jl_qFKXwPShk5 was explicitly requested, output will be shown live

[pid 21955] waiting for IO to finish:
 TYPE[FD/PID]       @UV_HANDLE_T->DATA
 timer              @0x600001585d00->0x10afffac0
  Activating project at `~/work/raicode3`            ]  0/1
Test Failed at /Users/nathandaly/work/jl_depots/raicode3/dev/Aqua/src/persistent_tasks.jl:98
  Expression: !(has_persistent_tasks(package, expr; kwargs...))

ERROR: There was an error during testing

@NHDaly NHDaly force-pushed the nhd-test-persistent-tasks-expr branch from 8be12e6 to d0cca64 Compare November 29, 2023 00:33
src/persistent_tasks.jl Outdated Show resolved Hide resolved
src/persistent_tasks.jl Outdated Show resolved Hide resolved
src/persistent_tasks.jl Outdated Show resolved Hide resolved
Copy link

codecov bot commented Nov 29, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (28d29dc) 75.20% compared to head (a112dd5) 75.20%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #255   +/-   ##
=======================================
  Coverage   75.20%   75.20%           
=======================================
  Files          11       11           
  Lines         750      750           
=======================================
  Hits          564      564           
  Misses        186      186           
Flag Coverage Δ
unittests 75.20% <100.00%> (ø)

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@NHDaly NHDaly marked this pull request as ready for review November 29, 2023 01:08
@lgoettgens
Copy link
Collaborator

lgoettgens commented Nov 29, 2023

I see where this is coming from and how this can be useful. Some comment from the first look: I would prefer expr to be a kwarg. In Aqua, we usually use kwargs for everything apart from the Package to test.
Furthermore, there are merge conflicts with #256 and #250. Could you have a look at those? I think most should be due to the docstring being mostly moved to a docs page.

@NHDaly NHDaly force-pushed the nhd-test-persistent-tasks-expr branch from 9041e85 to af5ffd7 Compare December 1, 2023 02:53
@NHDaly NHDaly force-pushed the nhd-test-persistent-tasks-expr branch from af5ffd7 to eabc74b Compare December 1, 2023 02:55
src/persistent_tasks.jl Outdated Show resolved Hide resolved
test/test_persistent_tasks.jl Outdated Show resolved Hide resolved
test/test_persistent_tasks.jl Outdated Show resolved Hide resolved
@NHDaly
Copy link
Member Author

NHDaly commented Dec 1, 2023

Okay! Thanks @lgoettgens.

I rebased on master, fixed the merge conflicts, added a test case, and changed expr to a keyword argument.

The reason I had made it a positional argument at first was because I wasn't sure what a good name should be. Is expr okay? Should it be precompile_expr?

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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.

Thanks for the changes! I have some minor comments, could you have a look at them? Once they are resolved, I'll update this PR for a version bump and do the merge/release procedure.

CHANGELOG.md Outdated Show resolved Hide resolved
docs/src/persistent_tasks.md Outdated Show resolved Hide resolved
src/persistent_tasks.jl Outdated Show resolved Hide resolved
src/persistent_tasks.jl Outdated Show resolved Hide resolved
@lgoettgens lgoettgens added the enhancement New feature or request label Dec 1, 2023
@lgoettgens
Copy link
Collaborator

The reason I had made it a positional argument at first was because I wasn't sure what a good name should be. Is expr okay? Should it be precompile_expr?

I have no real preference between the two.

Co-authored-by: Lars Göttgens <[email protected]>
src/persistent_tasks.jl Outdated Show resolved Hide resolved
src/persistent_tasks.jl Outdated Show resolved Hide resolved
Copy link
Member Author

@NHDaly NHDaly left a comment

Choose a reason for hiding this comment

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

Okay I think this is all set! Thanks for the nice review 😊 ❤️

src/persistent_tasks.jl Outdated Show resolved Hide resolved
Co-authored-by: Lars Göttgens <[email protected]>
@NHDaly
Copy link
Member Author

NHDaly commented Dec 1, 2023

Thanks! Should be all done then

@NHDaly NHDaly requested a review from lgoettgens December 1, 2023 15:35
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.

Thanks for working on this!

@lgoettgens lgoettgens enabled auto-merge (squash) December 1, 2023 15:39
@lgoettgens lgoettgens merged commit 34a1a02 into master Dec 1, 2023
23 checks passed
@lgoettgens lgoettgens deleted the nhd-test-persistent-tasks-expr branch December 1, 2023 15:43
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.

2 participants