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

Misleading test coverage report #5445

Closed
5 tasks
fjetter opened this issue Oct 20, 2021 · 1 comment
Closed
5 tasks

Misleading test coverage report #5445

fjetter opened this issue Oct 20, 2021 · 1 comment
Labels
good first issue Clearly described and easy to accomplish. Good for beginners to the project.

Comments

@fjetter
Copy link
Member

fjetter commented Oct 20, 2021

As a dask maintainer, I want to trust the code coverage report.

Our coverage badge is a bit misleading showing coverage below 90%. This is due to us not collecting coverage in a few places. Also, we simply have a few modules which are only there for debugging and/or historical reasons

The most relevant parts (scheduler, worker, etc.) do have quite good coverage. I believe the <90% batch doesn't reflect well on the project and the wrong configuration creates a lot of noise making it harder to see relevant gaps (there are quite a few)

Things I spot immediately wehich we should exclude/ignore (non exhaustive)

  • Exclude nvml calls
  • Exclude UCX
  • Exclude LOG_PDB brances
  • Ignore pytest_resourceleaks
  • Ignore _version.py (vendored)

Pragma no cover

  • We have cases with relatively long switch-like statements ending in a final exception, e.g. ValueError "unknown value"
  • Conditional imports may need be covered with pragma statements and/or a dedicated job

Relevant documentation https://coverage.readthedocs.io/en/coverage-4.3.3/excluding.html I would hope us being able to do this without code modification, if possible. A proper coverage.rc should be sufficient

There are other modules which we are not entirely sure. If it is not straight forward, this can be discussed in the PR review, e.g.

  • distributed/deploy/old_ssh.py
  • distributed/cli/dask_ssh.py
  • distributed/nanny.py

Long term goal:

  • We should have 100% test coverage
  • If there are weird edge cases where an extremely complicated test would be required to test an unimportant test case. For these cases we want to use deliberate nopragma instructions

AC

  • All very obvious cases should have a pragma statement and/or exclude configuration option
  • Debatable cases should still be left uncovered and be handled in a follow up ticket
  • Developer documentation is updated with a guideline
@fjetter fjetter added the good first issue Clearly described and easy to accomplish. Good for beginners to the project. label Oct 20, 2021
@fjetter fjetter assigned fjetter and unassigned fjetter Dec 16, 2021
@fjetter
Copy link
Member Author

fjetter commented Feb 15, 2022

The most important files are

  • node.py
  • core.py
  • scheduler.py
  • worker.py
  • client.py
  • stealing.py

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Clearly described and easy to accomplish. Good for beginners to the project.
Projects
None yet
Development

No branches or pull requests

2 participants