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

Split tests into groups + SafeTestsets #30

Merged
merged 13 commits into from
Dec 4, 2024
Merged

Conversation

lkdvos
Copy link
Collaborator

@lkdvos lkdvos commented Dec 4, 2024

This PR reorganizes the test structure as described in #28 .

The grouping logic happens automatically based on file structure:

  • Tests without a group, in the top folder are run as part of the group "top"
    test/mytest1.jl
    test/mytest2.jl
  • Tests within a folder are assigned to the group based on the folder name:
    test/utility/test1.jl
    test/basics/test2.jl
  • Examples are run as part of the group "examples"
    examples/myexample.jl

To select/run a subset of the tests, the Tests.yml CI can make use of the group input. Selecting this in a matrix ensures parallelism through the github runners.

To select/run a subset of the tests locally, the test_args keyword, along with the --group=groupname syntax can be used, for example:

using Pkg: Pkg
Pkg.test("mypackage"; test_args=`--group=examples`)

Note that all group name matches are insensitive to capitalization. The default group ALL runs all tests.

@mtfishman
Copy link
Member

Nice, I really like the idea of basing the groups off of the folder structure!

@mtfishman
Copy link
Member

Does this also work with the environment variable? Or did you decide not to go that route?

Copy link

codecov bot commented Dec 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 6.02%. Comparing base (9bc0e97) to head (f13bc27).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #30       +/-   ##
==========================================
- Coverage   85.54%   6.02%   -79.52%     
==========================================
  Files           1       1               
  Lines          83      83               
==========================================
- Hits           71       5       -66     
- Misses         12      78       +66     
Flag Coverage Δ
docs 0.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.

end,
)

@time begin
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't @testset report times now?

Copy link
Member

Choose a reason for hiding this comment

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

I.e. could we achieve this by wrapping all of this in @testset or @safetestset?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not entirely sure, I mostly copied this from my other workflows. I think the main difference is that @time also reports splits in compiletime/runtime, which I find interesting, but I have absolutely no preference about this.
Just as an aside, wrapping things in nested @testset structures makes it such that output is only printed after all tests finish. Not a big deal, but something to keep in mind.

@lkdvos
Copy link
Collaborator Author

lkdvos commented Dec 4, 2024

This works both with environment variables and test args, it first checks the latter, if this is empty it checks the environment variable, if that is empty it runs all tests

@lkdvos
Copy link
Collaborator Author

lkdvos commented Dec 4, 2024

(TODO: codecov token)

@lkdvos lkdvos requested a review from mtfishman December 4, 2024 16:32
@mtfishman
Copy link
Member

I think the conclusions from our discussion were:

  1. Enforce a convention that test files are named test_....jl.
  2. Have a convention for a certain folder that can't be used as a group where test utilities can go.

@lkdvos
Copy link
Collaborator Author

lkdvos commented Dec 4, 2024

If the tests pass, I would merge this and I'll add the setup folder/files in a separate PR.

@mtfishman
Copy link
Member

Thanks!

@mtfishman mtfishman merged commit 635f294 into ITensor:main Dec 4, 2024
10 of 11 checks passed
@lkdvos lkdvos deleted the ld-tests branch December 4, 2024 22:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants