[WIP] add pragma statements #5749

scharlottej13 · 2022-02-03T00:08:42Z

partially addresses Misleading test coverage report #5445, specifically:

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

WIP, because I haven't made it through all the files yet. Going in order of lowest to highest percent coverage I made it to utils.py.

cc: @ncclementi @phobson-- thanks again for your help!

…coverage

…nto test-coverage

add pragma no cover to profile add pragma no cover to utils_test taking another pass at pragma no cover for utils_test excluding obvious cases with pragma statement

adding obvious pragma no cover

…coverage

GPUtester · 2022-02-03T00:08:44Z

Can one of the admins verify this patch?

scharlottej13 · 2022-02-03T00:21:11Z

Files reviewed in this PR:

asyncio.py

_concurrent_futures_thread.py-- really not sure about this one! seems a lot is not covered, and I wasn't sure how easy this is to test.

proctitle.py

node.py-- wasn't sure about start_services, didn't add any pragma

objects.py-- didn't see any obvious place to add pragma here

actor.py

profile.py

line 110
line 368
lines 444-503 (how hard is it to test these?)

metrics.py

utils_test.py-- This one was tricky and perhaps should be removed from this PR to be part of a separate discussion (per the note in the original issue)? When unsure, I added pragma no cover where the only reference was in a test that is already ignored in .coveragerc; if the function was called elsewhere in utils_test.py, I opted to not exclude it. For some functions, I had trouble finding a reference, e.g deep, slowdouble. moved to #5799

github-actions · 2022-02-03T02:51:29Z

Unit Test Results

      17 files -       1       17 suites - 1 9h 47m 49s ⏱️ - 36m 22s
  2 596 tests ±      0   2 516 ✔️ -       1     79 💤 ±    0 1 ❌ +1
21 846 runs - 1 384 20 482 ✔️ - 1 276 1 363 💤 - 109 1 ❌ +1

For more details on these failures, see this check.

Results for commit 51f26ad. ± Comparison against base commit 91f899e.

♻️ This comment has been updated with latest results.

fjetter · 2022-02-03T09:54:04Z

Thank you for your effort on this! I would actually recommend opening the other files in another PR, doing this incrementally in batches. That makes reviewing a bit easier and we can merge incrementally to avoid merge conflicts

node.py-- wasn't sure about start_services

I think this is a good example for a thing we do not want to ignore. We are clearly entering this function (the if and for is hit) but for some reason the actual logic is never executed. We likely want to spend some time figuring out why this is and maybe wrote a test for it.

his one was tricky and perhaps should be removed from this PR to be part of a separate discussion

If you are not sure about this, feel free to split it off into a separate PR. In my mind the initial PRs for this should be fast and without a lot of discussion

line 110
line 368

I suggest to not ignore. I don't know how this is tested right now but both lines look like actual functionality which we should be testing imo

lines 444-503 (how hard is it to test these?)

They are hit from what I see. If you have reports were they are not hit, this is one of these "flaky" coverage places where it might pay off to have a closer look so I suggest to not ignore

fjetter

I agree that we should split off the utils.py to another PR. I think a lot of these ignores should not be there and this may be an opportunity to clean up our API a little bit

fjetter · 2022-02-03T09:58:05Z

distributed/actor.py

@@ -143,7 +143,7 @@ def __getattr__(self, key):
                return lambda *args, **kwargs: ActorFuture(
                    None, self._io_loop, result=attr(*args, **kwargs)
                )
-            else:
+            else:  # pragma: no cover


getattr is nasty and imo should be properly tested even for trivial cases

Suggested change

else: # pragma: no cover

else:

fjetter · 2022-02-03T09:58:36Z

distributed/cli/dask_scheduler.py

@@ -218,4 +218,4 @@ def go():


 if __name__ == "__main__":
-    go()


I'm wondering if the entire CLI module should be ignored. Can we get reliable coverage information from this?

fjetter · 2022-02-03T09:59:36Z

distributed/deploy/adaptive_core.py

 if TYPE_CHECKING:
-    from ..scheduler import WorkerState
+    from ..scheduler import WorkerState  # pragma: no cover


I assume this can be added to

distributed/.coveragerc

Lines 16 to 25 in 27e467e

exclude_lines =

# re-enable the standard pragma

pragma: nocover

pragma: no cover

# exclude nvml calls

[\s(.]nvml[\s(.]

[\s(.]pynvml[\s(.]

# exclude LOG_PDB

LOG_PDB

such that TYPE_CHECKING blocks are always ignored

fjetter · 2022-02-03T10:00:44Z

distributed/deploy/spec.py

@@ -271,7 +271,7 @@ async def _start(self):
        self._lock = asyncio.Lock()
        self.status = Status.starting

-        if self.scheduler_spec is None:


keep this, please. This smells like dead code or something we should be testing

fjetter · 2022-02-03T10:03:07Z

distributed/profile.py

@@ -348,7 +348,7 @@ def get_profile(history, recent=None, start=None, stop=None, key=None):

    if stop is None:
        istop = None
-    else:


keep this please. Either it should be tested or it shouldn't be there.

fjetter · 2022-02-03T10:03:45Z

distributed/utils_test.py

    from typing_extensions import Literal

 from distributed.compatibility import MACOS
 from distributed.scheduler import Scheduler

 try:
    import ssl
-except ImportError:


we could consider adding except ImportError as well to the exclude_lines

scharlottej13 · 2022-02-10T01:32:20Z

Thank you for your effort on this! I would actually recommend opening the other files in another PR, doing this incrementally in batches. That makes reviewing a bit easier and we can merge incrementally to avoid merge conflicts

Thank you @fjetter for the review! Once I address your comments I'll open up separate PRs for the other files, starting with utils_test.py.

lines 444-503 (how hard is it to test these?)

They are hit from what I see. If you have reports were they are not hit, this is one of these "flaky" coverage places where it might pay off to have a closer look so I suggest to not ignore

Oh interesting, yup in the most recent coverage report I can see they are covered. I won't ignore them, then, and will also see if I can find the report where I saw them marked as not covered.

…nto improve-coverage

fjetter

Thank you @scharlottej13 !

scharlottej13 added 14 commits January 12, 2022 14:35

exclude nvml calls, explicitly include nvml and pynvml for clarity

3d2e040

exclude _version, pytest_resourceleaks, pdb

b0796c1

exclude UCX

ff7f5eb

Merge branch 'main' of https://github.com/dask/distributed into test-…

dec54c9

…coverage

more inclusive regex for nvml and pynvml

6e36774

don't exclude entire file

85833d3

add other versions of standard pragma

fcc2642

remove files that no longer exist

46c9618

Merge branch 'main' of https://github.com/scharlottej13/distributed i…

b57d7bf

…nto test-coverage

don't omit compatibility.py or utils_test.py

63321d1

Merge branch 'main' of https://github.com/scharlottej13/distributed i…

84a560c

…nto test-coverage

exclude obvious cases with pragma statement

6207e40

add pragma no cover to profile add pragma no cover to utils_test taking another pass at pragma no cover for utils_test excluding obvious cases with pragma statement

adding files to coveragerc for exclusion

9951196

adding obvious pragma no cover

Merge branch 'main' of https://github.com/dask/distributed into test-…

3da2d54

…coverage

fjetter reviewed Feb 3, 2022

View reviewed changes

scharlottej13 added 7 commits February 10, 2022 08:32

fix merge conflict

8f51b24

always exclude type checking from coverage

c19da45

removing pragma

218f3cc

exclude except ImportError (for discussion)

5eb6168

remove pragma from utils_test, will open in a separate PR

343e124

Merge branch 'main' of https://github.com/scharlottej13/distributed i…

9fec8ee

…nto improve-coverage

remove pragma statements now covered by .coveragerc

5b0bbc9

This was referenced Feb 11, 2022

add pragma - utils_test #5799

Open

add pragma - cli files #5800

Merged

moving cli exclusions to new PR

07226ca

move to new PR

51f26ad

fjetter approved these changes Feb 15, 2022

View reviewed changes

fjetter marked this pull request as ready for review February 15, 2022 10:29

fjetter merged commit 9f8c50b into dask:main Feb 15, 2022

scharlottej13 deleted the improve-coverage branch February 15, 2022 18:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] add pragma statements #5749

[WIP] add pragma statements #5749

scharlottej13 commented Feb 3, 2022 •

edited

Loading

GPUtester commented Feb 3, 2022

scharlottej13 commented Feb 3, 2022 •

edited

Loading

github-actions bot commented Feb 3, 2022 •

edited

Loading

fjetter commented Feb 3, 2022

fjetter left a comment

fjetter Feb 3, 2022

fjetter Feb 3, 2022

fjetter Feb 3, 2022

fjetter Feb 3, 2022

fjetter Feb 3, 2022

fjetter Feb 3, 2022

scharlottej13 commented Feb 10, 2022

fjetter left a comment

	exclude_lines =
	# re-enable the standard pragma
	pragma: nocover
	pragma: no cover
	# exclude nvml calls
	[\s(.]nvml[\s(.]
	[\s(.]pynvml[\s(.]
	# exclude LOG_PDB
	LOG_PDB

[WIP] add pragma statements #5749

[WIP] add pragma statements #5749

Conversation

scharlottej13 commented Feb 3, 2022 • edited Loading

GPUtester commented Feb 3, 2022

scharlottej13 commented Feb 3, 2022 • edited Loading

github-actions bot commented Feb 3, 2022 • edited Loading

Unit Test Results

fjetter commented Feb 3, 2022

fjetter left a comment

Choose a reason for hiding this comment

fjetter Feb 3, 2022

Choose a reason for hiding this comment

fjetter Feb 3, 2022

Choose a reason for hiding this comment

fjetter Feb 3, 2022

Choose a reason for hiding this comment

fjetter Feb 3, 2022

Choose a reason for hiding this comment

fjetter Feb 3, 2022

Choose a reason for hiding this comment

fjetter Feb 3, 2022

Choose a reason for hiding this comment

scharlottej13 commented Feb 10, 2022

fjetter left a comment

Choose a reason for hiding this comment

scharlottej13 commented Feb 3, 2022 •

edited

Loading

scharlottej13 commented Feb 3, 2022 •

edited

Loading

github-actions bot commented Feb 3, 2022 •

edited

Loading