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 a single ci job per group with depwarn=error #2956

Merged
merged 4 commits into from
Oct 27, 2023
Merged

Conversation

lgoettgens
Copy link
Member

The idea came first up in #2693 (comment), and now with #2913 present, I think this is a good time to talk about this proposal.
And due to #2913 not merged, we have the chance to test this PR.

@lgoettgens lgoettgens added the testsuite Everything concerning testsuite label Oct 26, 2023
@lgoettgens lgoettgens mentioned this pull request Oct 26, 2023
@lgoettgens
Copy link
Member Author

With this change (if it works), the large part of the testsuite ignores deprecation warnings. These are only catches by the new few jobs.
This helps us to still have the large part of the testsuite working when a dependency deprecated something.

@thofma
Copy link
Collaborator

thofma commented Oct 26, 2023

OK, I see. Would also be fine with me. Compared to testing deprecations in the downstream tests via OscarDevTools, the solution here is better. What do you think @benlorenz?

@benlorenz
Copy link
Member

I think separating the deprecations check is better than enabling depwarn for OscarDevTools.

But we need to run doctests with depwarn as well.

@codecov
Copy link

codecov bot commented Oct 26, 2023

Codecov Report

Merging #2956 (ac1a185) into master (b6c8dc8) will decrease coverage by 0.06%.
Report is 1 commits behind head on master.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2956      +/-   ##
==========================================
- Coverage   80.41%   80.35%   -0.06%     
==========================================
  Files         470      470              
  Lines       66601    66648      +47     
==========================================
  Hits        53556    53556              
- Misses      13045    13092      +47     

see 8 files with indirect coverage changes

@lgoettgens
Copy link
Member Author

The jobs are now named correctly. But I expected the depwarn=error tests to fail due to #2913 not merged on this branch.
@benlorenz do you have an idea what's going on?

@benlorenz
Copy link
Member

benlorenz commented Oct 26, 2023

The tests run on a temporary merge commit with the current master, so this includes the fix now. The test run before the job / doctest adaptions did fail due to the deprecations for julia 1.9: https://github.com/oscar-system/Oscar.jl/actions/runs/6649926182/job/18069184829

Maybe we can use something like depwarn: 'depwarn' for the matrix and then in the job:

${{ matrix.depwarn == 'depwarn' && 'error' || 'no' }}

This way the label for the job would contain depwarn instead of just error.

And we should probably adjust either the codecov upload count or whether the new jobs do uploads. The codecov page for the current commit shows 6 uploads: https://app.codecov.io/gh/oscar-system/Oscar.jl/commit/ac1a1855de32b2f104bff0f088105292c5e61393.

@lgoettgens
Copy link
Member Author

I decided to name it depwarn=error to make clear that deprecations actually error and not just warn.

Furthermore, I disabled codecov for these new jobs. If we expect the to fail kind of often, it would be senseless to add the to the codecov config and wait infinitely for the comments. And they shouldn't bring any new covered lines, or at least not a significant amount.

@benlorenz
Copy link
Member

The run command for the doctests still has an unconditional --depwarn=error (currently line 138):
https://github.com/oscar-system/Oscar.jl/pull/2956/files#diff-3ab46ee209a127470fce3c2cf106b1a1dbadbb929a4b5b13656a4bc4ce19c0b8R138

(The with block a few lines above is only for running the normal tests on macos, which is done in the doctests job because we only have 5 parallel macos runners)

@lgoettgens
Copy link
Member Author

Oh yeah, true. Thanks for spotting! Should be fixed now.

Copy link
Member

@benlorenz benlorenz left a comment

Choose a reason for hiding this comment

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

thanks!

@benlorenz benlorenz merged commit 8a53dde into master Oct 27, 2023
15 of 20 checks passed
@benlorenz benlorenz deleted the lgoettgens-patch-1 branch October 27, 2023 09:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testsuite Everything concerning testsuite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants