Update pinned pytest version to 6.2.1 and fix error in test_exceptions.py::test_python_alreadyset_in_destructor by YannickJadoul · Pull Request #2741 · pybind/pybind11

YannickJadoul · 2020-12-23T23:09:48Z

Description

As noted in #2573 (comment), pytest 6 seems to make one of our tests fail. Let's check this, and then figure out if/how to fix that.

Suggested changelog entry:

* Fix issue with a test failing on PyTest 6.2.

henryiii · 2020-12-23T23:30:17Z

I think we just need to add a marker to turn this extra pytest feature off for this test. We do want this behavior.

I noticed the dropping of 3.5 in the changelog.

YannickJadoul · 2020-12-23T23:42:47Z

It would be nice if we could explicitly turn it off for just that test, though, and not for everything, with the blunt -p no:unraisableexception flags?

@pytest.mark.filterwarnings("ignore::pytest.PytestUnraisableExceptionWarning") ignores the warning, but still fails our test. Presumably because pytest holds sys.unraisablehook hostage, and doesn't propagate the behavior to the default implemenation, on which we depend in the test.

bstaletic · 2020-12-23T23:46:57Z

Can we use the monkeypatch fixture to force our hook and then restore pytest's hook?

…k__ instead of sys.unraisablehook

YannickJadoul · 2020-12-23T23:48:30Z

Got something. The original built-in sys.unraisablehook is kept as sys.__unraisablehook__, so we can just use that?

henryiii · 2020-12-23T23:56:06Z

Why make it a string but still error out if unfound, PyTest... needs an if to see if that exception exists.

YannickJadoul · 2020-12-24T00:06:02Z

+ "ignore::pytest.PytestUnraisableExceptionWarning"
+ )
+else:
+ ignore_pytest_unraisable_warning = lambda f: f # NOQA: E731


Yes, I could not put a lambda, and put 2 lines def ignore_pytest_unraisable_warning(f): and return f, but then black suddenly adds an empty line after else:, so that's just as ugly.

YannickJadoul · 2020-12-24T00:06:36Z

Why make it a string but still error out if unfound, PyTest... needs an if to see if that exception exists.

Yeah, indeed ... :-/ Guess they're just following the warnings module?

YannickJadoul · 2020-12-24T00:08:54Z

 assert d["good"] is True


+if hasattr(pytest, "PytestUnraisableExceptionWarning"): # Python >= 3.8 and pytest >= 6


Alternatively, we move this into the function, and directly use the warnings module. But I'm not sure whether such a filter would get reset after the test or not, this way.

YannickJadoul · 2020-12-24T00:31:17Z

Green! :-) Ready to be reviewed.

bstaletic

Like I said "a decorator factory" could be a fancy way of avoiding code in global scope, but not a blocker.

YannickJadoul · 2020-12-24T01:40:16Z

 hooked = True
- default_hook = sys.unraisablehook
+ # Don't take `sys.unraisablehook`, as that's overwritten by pytest
+ default_hook = sys.__unraisablehook__


I take it we're happy with this?
If some case where this would be overwritten (by something else than pytest), we're ignoring it as well, ofc, but ... myeah, best I could immediately think of.

Yes, it seems fine to me. We don't want pytest's version here, and it's a test, so nothing else should be fiddling with it.

YannickJadoul · 2020-12-24T12:02:55Z

Does this need to be marked 2.6.2, still? I guess it's kind of independent from the releases, but ...

Guess it's mostly #2573 that needs to make sure it also installs tests/requirements.txt, as I noted there.

henryiii · 2020-12-24T14:57:37Z

Great! We don't really control the PyTest version if someone wants to run our tests (a requirements.txt is an internal pinning, not an external one), and a few of our tests use system PyTests, but it's good not to be (too) version specific, tests failing due to PyTest is not ideal.

YannickJadoul · 2020-12-24T15:01:23Z

Do you want a changelog for this, @henryiii?

henryiii · 2020-12-24T15:11:08Z

Yes, I added it to the description.

Here it is in setuptools changelog, for example: https://setuptools.readthedocs.io/en/latest/history.html#misc

YannickJadoul · 2020-12-24T19:46:13Z

Yes, I added it to the description.

Great! Hadn't spotted that; thanks! :-)

Update pytest to 6.2.1 in tests/requirements.txt
615eeea

YannickJadoul marked this pull request as draft December 23, 2020 23:10

Pin pytest to last supported version for 3.5
b456a82

YannickJadoul mentioned this pull request Dec 23, 2020
CI: Intel icc/icpc via oneAPI #2573
Merged

Suppress PytestUnraisableExceptionWarning and use sys.__unraisablehoo…
008be88
…k__ instead of sys.unraisablehook

YannickJadoul changed the title ~~Test (and hopefully soon fix) test failure with pytest 6~~Update pinned pytest version to 6 and fix error in test_exceptions.py::test_python_alreadyset_in_destructorDec 23, 2020

YannickJadoul marked this pull request as ready for review December 23, 2020 23:49

YannickJadoul changed the title ~~Update pinned pytest version to 6 and fix error in test_exceptions.py::test_python_alreadyset_in_destructor~~Update pinned pytest version to 6.2.1 and fix error in test_exceptions.py::test_python_alreadyset_in_destructorDec 23, 2020

Fix filterwarnings mark on old pytest and old Python versions
df68528

YannickJadoul commented Dec 24, 2020
View reviewed changes

bstaletic reviewed Dec 24, 2020
View reviewed changes

Comment threadtests/test_exceptions.py Outdated

Cleanup ignore_pytest_unraisable_warning decorator
f21651a

YannickJadoul commented Dec 24, 2020
View reviewed changes

henryiii added this to the v2.6.2 milestone Dec 24, 2020

henryiii approved these changes Dec 24, 2020
View reviewed changes

henryiii merged commit 830f8ed into pybind:masterDec 24, 2020

github-actionsBot added the needs changelog Possibly needs a changelog entry label Dec 24, 2020

bstaletic mentioned this pull request Dec 24, 2020
[BUG] Memory leaks when casting raw pointers to python types #2742
Open

YannickJadoul deleted the pytest-6 branch December 24, 2020 15:01

henryiii removed the needs changelog Possibly needs a changelog entry label Dec 29, 2020

henryiii mentioned this pull request Jan 26, 2021
chore: prepare for the 2.6.2 release #2821
Merged

rwgk mentioned this pull request Feb 10, 2023
FWD pybind11 google/pybind11clif#2741
Closed

		assert d["good"] is True


		if hasattr(pytest, "PytestUnraisableExceptionWarning"): # Python >= 3.8 and pytest >= 6

Uh oh!

Conversation

YannickJadoul commented Dec 23, 2020• edited by henryiii Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Suggested changelog entry:

Uh oh!

henryiii commented Dec 23, 2020

Uh oh!

YannickJadoul commented Dec 23, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bstaletic commented Dec 23, 2020

Uh oh!

YannickJadoul commented Dec 23, 2020

Uh oh!

henryiii commented Dec 23, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

YannickJadoulDec 24, 2020

Choose a reason for hiding this comment

Uh oh!

YannickJadoul commented Dec 24, 2020

Uh oh!

YannickJadoulDec 24, 2020

Choose a reason for hiding this comment

Uh oh!

YannickJadoul commented Dec 24, 2020

Uh oh!

bstaletic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

YannickJadoulDec 24, 2020

Choose a reason for hiding this comment

Uh oh!

henryiiiDec 24, 2020

Choose a reason for hiding this comment

Uh oh!

YannickJadoul commented Dec 24, 2020

Uh oh!

henryiii commented Dec 24, 2020

Uh oh!

YannickJadoul commented Dec 24, 2020

Uh oh!

henryiii commented Dec 24, 2020

Uh oh!

YannickJadoul commented Dec 24, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

YannickJadoul commented Dec 23, 2020•
edited by henryiii
Loading

YannickJadoul commented Dec 23, 2020•
edited
Loading

henryiii commented Dec 23, 2020•
edited
Loading