apply warnings filter as soon as possible, and remove it as late as possible #13057

graingert · 2024-12-13T08:01:13Z

…ossible

…o gc.collect() is called with the warning filters in place

graingert · 2024-12-13T08:01:56Z

src/_pytest/config/__init__.py

@@ -1112,9 +1112,7 @@ def add_cleanup(self, func: Callable[[], None]) -> None:
    def _do_configure(self) -> None:
        assert not self._configured
        self._configured = True
-        with warnings.catch_warnings():


not sure what this was for - it just disables the user specified warning filter during config. We don't want this.

for more information, see https://pre-commit.ci

pyproject.toml

jakkdl · 2024-12-16T15:58:38Z

Wait, test_create_task_unraisable and test_create_task_unraisable_warning_filter has 100% the exact same behavior?

Would be nice with a test for unawaited coroutine, something like #10404 (comment)

Can perhaps also add a test that hits PytestFDWarning to get 100% patch coverage

graingert · 2024-12-16T16:28:50Z

Wait, test_create_task_unraisable and test_create_task_unraisable_warning_filter has 100% the exact same behavior?

No, test_create_task_unraisable relies on the warning filter set by the host test suite
In test_create_task_unraisable_warning_filter I disable the host pytest warning filter, and configure the child pytest warning filter

Would be nice with a test for unawaited coroutine, something like #10404 (comment)

I make my own object that behaves like an unawaited coro, I don't think it's needed

Can perhaps also add a test that hits PytestFDWarning to get 100% patch coverage

CI absolutely covers this case - without it CI fails. I suspect coverage isn't running on the lsof tests

jakkdl · 2024-12-17T09:25:25Z

Wait, test_create_task_unraisable and test_create_task_unraisable_warning_filter has 100% the exact same behavior?

No, test_create_task_unraisable relies on the warning filter set by the host test suite In test_create_task_unraisable_warning_filter I disable the host pytest warning filter, and configure the child pytest warning filter

The only line that differs is whether you do pytester.runpytest() vs pytester.runpytest('-Werror'), yeah? If so I think you should at least parametrize the test (or run a loop, or something). And a test with -Wdefault` or something where the outcome of the test changes.

Would be nice with a test for unawaited coroutine, something like #10404 (comment)

I make my own object that behaves like an unawaited coro, I don't think it's needed

It might just be me, but skimming at #10404 + the test I need to spend a minute to figure out how the test functions as a regression test for it. So IMO at least a comment, but would personally have a test that is more obviously equivalent to the issue.

Can perhaps also add a test that hits PytestFDWarning to get 100% patch coverage

CI absolutely covers this case - without it CI fails. I suspect coverage isn't running on the lsof tests

in this case it's not GitHub annotations lying, you don't have 100% patch coverage
https://app.codecov.io/gh/pytest-dev/pytest/pull/13057?dropdown=coverage&src=pr&el=h1
You'd only get a fail if it was lower than the target of 96.32%, and you have 97.14%.
If it's a case of tests not being run in CI then I suppose a # pragma: no cover?

graingert · 2024-12-17T09:29:04Z

Wait, test_create_task_unraisable and test_create_task_unraisable_warning_filter has 100% the exact same behavior?

No, test_create_task_unraisable relies on the warning filter set by the host test suite In test_create_task_unraisable_warning_filter I disable the host pytest warning filter, and configure the child pytest warning filter

The only line that differs is whether you do pytester.runpytest() vs pytester.runpytest('-Werror'), yeah? If so I think you should at least parametrize the test (or run a loop, or something). And a test with -Wdefault` or something where the outcome of the test changes.

This shouldn't be the case, the filterwarnings mark is also different - it disables the host pytest warning filter

Would be nice with a test for unawaited coroutine, something like #10404 (comment)

I make my own object that behaves like an unawaited coro, I don't think it's needed

It might just be me, but skimming at #10404 + the test I need to spend a minute to figure out how the test functions as a regression test for it. So IMO at least a comment, but would personally have a test that is more obviously equivalent to the issue.

Ok, I suppose it does makes sense to do a more obvious regression test

Can perhaps also add a test that hits PytestFDWarning to get 100% patch coverage

CI absolutely covers this case - without it CI fails. I suspect coverage isn't running on the lsof tests

app.codecov.io/gh/pytest-dev/pytest/pull/13057?dropdown=coverage&src=pr&el=h1

Yeah, what I mean to say is that CI fails without this change because the warning previously was issued too late and so wouldn't trigger an error. Now that it's correctly raised as an error CI fails unless I explicitly ignore it, hence giving it a new type

jakkdl · 2024-12-17T09:34:58Z

Uh, sorry for not doing a threaded convo as a review. Don't ask me why I didn't.

This shouldn't be the case, the filterwarnings mark is also different - it disables the host pytest warning filter

Ah, derp. I guess a comment or two explaining how they differ would help other people as blind as me.

Yeah, what I mean to say is that CI fails without this change because the warning previously was issued too late and so wouldn't trigger an error. Now that it's correctly raised as an error CI fails unless I explicitly ignore it, hence giving it a new type

right, yeah. Weird it doesn't show up as being covered then. Maybe only hit in a subprocess pytester invokation? If so we can perhaps add a more explicit test case for it

changelog/10404.bugfix.rst

graingert · 2024-12-17T10:29:21Z

@jakkdl ok I've pushed a new test and some comments

graingert · 2024-12-17T11:06:56Z

@jakkdl also enabling coverage on lsof tests seems to fix the missing coverage

jakkdl

Would be nice if one test (or a separate test) doesn't run with -Werror, but otherwise looks great.

jakkdl · 2024-12-17T11:57:34Z

testing/test_unraisableexception.py

+    with _disable_gc():
+        result = pytester.runpytest("-Werror")
+
+    # TODO: should be a test failure or error
+    assert result.ret == pytest.ExitCode.INTERNAL_ERROR
+
+    result.assert_outcomes(passed=1)
+    result.stderr.fnmatch_lines("RuntimeWarning: coroutine 'my_task' was never awaited")


If we run this without -Werror we can check that normal warning functionality works.

ah I broke it! Very good catch! I'll send a commit to fix it now

…nings

jakkdl · 2024-12-18T09:37:13Z

src/_pytest/warnings.py

-    with warnings.catch_warnings(record=True) as log:
-        # mypy can't infer that record=True means log is not None; help it.
-        assert log is not None
-


Looking at typeshed I think the comment was previously incorrect and the assert wasn't needed https://github.com/python/typeshed/blob/54e1c6ad58961c1faebc6842698eec78f1298f92/stdlib/warnings.pyi#L82
But ye with current logic mypy can't infer if record -> log is not None later in the func

testing/test_unraisableexception.py

graingert added 3 commits December 13, 2024 07:58

apply warnings filter as soon as possible, and remove it as late as p…

fef1f09

…ossible

apply unraisablehook and threadexception before the warning plugin, s…

229a64b

…o gc.collect() is called with the warning filters in place

apply the warning filter during pytest_configure

10eb342

graingert commented Dec 13, 2024

View reviewed changes

pre-commit-ci bot and others added 4 commits December 13, 2024 08:04

[pre-commit.ci] auto fixes from pre-commit.com hooks

11714dd

for more information, see https://pre-commit.ci

restore the filterwarnings marker config

cc5b555

move lsof warning to something ignorable and ignore it

a3881db

add newsfragment

f37d07a

psf-chronographer bot added the bot:chronographer:provided (automation) changelog entry is part of PR label Dec 13, 2024

graingert and others added 2 commits December 13, 2024 08:30

add test

9a953f4

[pre-commit.ci] auto fixes from pre-commit.com hooks

7581137

for more information, see https://pre-commit.ci

graingert commented Dec 13, 2024

View reviewed changes

pyproject.toml Outdated Show resolved Hide resolved

graingert added 3 commits December 13, 2024 08:31

simplify PytestFDWarning filter

b32316c

fix PytestFDWarning module

72475f4

get more patch coverage

e2e81b2

graingert marked this pull request as ready for review December 13, 2024 10:30

graingert requested a review from jakkdl December 13, 2024 10:31

Merge branch 'main' into apply-warnings-filter-asap-remove-alap

5349709

Merge branch 'main' into apply-warnings-filter-asap-remove-alap

89ff09c

graingert commented Dec 17, 2024

View reviewed changes

changelog/10404.bugfix.rst Show resolved Hide resolved

graingert added 3 commits December 17, 2024 09:47

Update changelog/10404.bugfix.rst

b5e00fb

add more realistic create_task/unawaited coro warning test

a00679a

update comments

e828fa9

enable coverage on lsof tests

003a51f

jakkdl approved these changes Dec 17, 2024

View reviewed changes

ensure global warning filter application does not swallow default war…

808ad3d

…nings

graingert requested a review from jakkdl December 18, 2024 06:48

jakkdl reviewed Dec 18, 2024

View reviewed changes

graingert commented Dec 18, 2024

View reviewed changes

testing/test_unraisableexception.py Show resolved Hide resolved

Update testing/test_unraisableexception.py

c5fb158

graingert requested a review from jakkdl December 18, 2024 09:52

jakkdl approved these changes Dec 18, 2024

View reviewed changes

jakkdl requested a review from nicoddemus December 18, 2024 10:58

graingert merged commit 868e1d2 into main Dec 18, 2024
28 checks passed

graingert deleted the apply-warnings-filter-asap-remove-alap branch December 18, 2024 11:09

graingert mentioned this pull request Dec 19, 2024

work out what to do with very late and very early warnings miketheman/pytest-max-warnings#2

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

apply warnings filter as soon as possible, and remove it as late as possible #13057

apply warnings filter as soon as possible, and remove it as late as possible #13057

graingert commented Dec 13, 2024

graingert Dec 13, 2024

jakkdl commented Dec 16, 2024

graingert commented Dec 16, 2024 •

edited

Loading

jakkdl commented Dec 17, 2024 •

edited

Loading

graingert commented Dec 17, 2024 •

edited

Loading

jakkdl commented Dec 17, 2024

graingert commented Dec 17, 2024

graingert commented Dec 17, 2024

jakkdl left a comment

jakkdl Dec 17, 2024

graingert Dec 18, 2024

jakkdl Dec 18, 2024

apply warnings filter as soon as possible, and remove it as late as possible #13057

apply warnings filter as soon as possible, and remove it as late as possible #13057

Conversation

graingert commented Dec 13, 2024

graingert Dec 13, 2024

Choose a reason for hiding this comment

jakkdl commented Dec 16, 2024

graingert commented Dec 16, 2024 • edited Loading

jakkdl commented Dec 17, 2024 • edited Loading

graingert commented Dec 17, 2024 • edited Loading

jakkdl commented Dec 17, 2024

graingert commented Dec 17, 2024

graingert commented Dec 17, 2024

jakkdl left a comment

Choose a reason for hiding this comment

jakkdl Dec 17, 2024

Choose a reason for hiding this comment

graingert Dec 18, 2024

Choose a reason for hiding this comment

jakkdl Dec 18, 2024

Choose a reason for hiding this comment

graingert commented Dec 16, 2024 •

edited

Loading

jakkdl commented Dec 17, 2024 •

edited

Loading

graingert commented Dec 17, 2024 •

edited

Loading