Skip to content

Add coverage to minimal dependency test#12193

Merged
pllim merged 1 commit into
astropy:mainfrom
nstarman:tst_cov
Sep 27, 2021
Merged

Add coverage to minimal dependency test#12193
pllim merged 1 commit into
astropy:mainfrom
nstarman:tst_cov

Conversation

@nstarman

@nstarman nstarman commented Sep 24, 2021

Copy link
Copy Markdown
Member

Also add coverage report to minimal dependency test.

Checklist for package maintainer(s)

This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.

  • Do the proposed changes actually accomplish desired goals?
  • Do the proposed changes follow the Astropy coding guidelines?
  • Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
  • Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
  • Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see "When to rebase and squash commits".
  • Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the Extra CI label.
  • Is a change log needed? If yes, did the change log check pass? If no, add the no-changelog-entry-needed label. If this is a manual backport, use the skip-changelog-checks label unless special changelog handling is necessary.
  • Is a milestone set? Milestone must be set but astropy-bot check might be missing; do not let the green checkmark fool you.
  • At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate backport-X.Y.x label(s) before merge.

@nstarman nstarman added this to the v5.0 milestone Sep 24, 2021
@nstarman nstarman requested review from dhomeier and pllim September 24, 2021 21:03
@pllim pllim requested a review from a team September 24, 2021 21:05
@pllim

pllim commented Sep 24, 2021

Copy link
Copy Markdown
Member

Thanks, I'll have a look next week but people who are more familiar with codecov are welcome to review too!

@dhomeier

Copy link
Copy Markdown
Contributor

Same here!

@astrofrog

Copy link
Copy Markdown
Member

Just to check, what is the motivation for this? Ideally we would measure coverage on all jobs but we don't for performance reasons as it adds a significant overhead. I'm curious what we gain by adding it to this job?

@pllim

pllim commented Sep 24, 2021

Copy link
Copy Markdown
Member

There was confusion when a test that only runs in a minimal dependency setting gets reported as not covered even though there is a test.

@dhomeier

Copy link
Copy Markdown
Contributor

#12192 (comment)

@astrofrog

Copy link
Copy Markdown
Member

Ok thanks for the background! Maybe a comment should be added in the CI config to explain this.

Is the addition of -clocale needed? If not, maybe just add -cov?

@nstarman

Copy link
Copy Markdown
Member Author

I think the clocale can be removed, at least #12192 (comment) does not require it.

Signed-off-by: Nathaniel Starkman (@nstarman) <nstarkman@protonmail.com>
@nstarman

Copy link
Copy Markdown
Member Author

Maybe a comment should be added in the CI config to explain this.

Done. Added a short note about running tests without scipy.

@pllim pllim left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. Let's deploy and see what happens.

@pllim pllim merged commit 43c11a2 into astropy:main Sep 27, 2021
@dhomeier

Copy link
Copy Markdown
Contributor

Does not look like the overhead is all that large – only a few minutes difference in testing times, at most.
I am a bit puzzled by the py38-test-alldeps job BTW, which is running with --open-args, but contrary to the comments does not seem to take any longer to run either.
By comparison, running astropy.test(open_files=True) locally takes over 1 hour compared to ~10 min without – or is that just another macOS thingy?

@pllim

pllim commented Sep 27, 2021

Copy link
Copy Markdown
Member

Wow, an hour! I am not sure... I don't think I ever tried to run open_files=True with the astropy test runner. I can try it on other OSes when I get the chance.

@nstarman nstarman deleted the tst_cov branch September 27, 2021 23:28
@pllim

pllim commented Sep 28, 2021

Copy link
Copy Markdown
Member

Yup, I see 1.5 hours on Linux. Must be the overhead of checking for dangling file pointers in the test runner. I don't see it being stuck on any one job. Not sure what we can do about that.

17767 passed, 201 skipped, 66 xfailed in 5241.37s (1:27:21)

@nstarman nstarman mentioned this pull request Oct 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants