Skip to content

Conversation

s-t-e-v-e-n-k
Copy link
Contributor

@s-t-e-v-e-n-k s-t-e-v-e-n-k commented Jun 20, 2024

Change Summary

With pytest 8.x, pytest.warns() now emits all warnings, bump the required version of pytest and then assert that all of them are the expected warning, rather than filtering them out.

Related issue number

Fixes #9597

Checklist

  • The pull request title is a good summary of the changes - it will be used in the changelog
  • Unit tests for the changes exist
  • Tests pass on CI
  • Documentation reflects the changes where applicable
  • My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

Selected Reviewer: @sydney-runkle

@github-actions github-actions bot added the relnotes-fix Used for bugfixes. label Jun 20, 2024
@s-t-e-v-e-n-k
Copy link
Contributor Author

please review

Copy link

codspeed-hq bot commented Jun 20, 2024

CodSpeed Performance Report

Merging #9702 will not alter performance

Comparing s-t-e-v-e-n-k:check-all-warnings-with-pytest (43f6641) with main (5a77c5c)

Summary

✅ 13 untouched benchmarks

Copy link
Contributor

@sydney-runkle sydney-runkle left a comment

Choose a reason for hiding this comment

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

Thanks for your awesome work here.

Looks like a few tests are still failing - perhaps we could still use context managers for each statement that warns, instead of catching all of the warnings in one place for each test. I think that might help iron out the current test failure as well!

Comment on lines 349 to 365
def test_config_class_is_deprecated(self):
with warnings.catch_warnings():
# we need to explicitly ignore the other warning in pytest-8
# TODO: rewrite it to use two nested pytest.warns() when pytest-7 is no longer supported
warnings.simplefilter('ignore')
with pytest.warns(
PydanticDeprecatedSince20,
match='Support for class-based `config` is deprecated, use ConfigDict instead.',
):

class Config(BaseConfig):
pass
with pytest.warns(PydanticDeprecatedSince20) as all_warnings:

class Config(BaseConfig):
pass

assert len(all_warnings) == 2
expected_warnings = [
'BaseConfig is deprecated. Use the `pydantic.ConfigDict` instead',
'Support for class-based `config` is deprecated, use ConfigDict instead',
]
assert [w.message.message for w in all_warnings] == expected_warnings

def test_config_class_attributes_are_deprecated(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like we're getting some failures for these two:

https://github.com/pydantic/pydantic/actions/runs/9590698692/job/26446460308?pr=9702

The changes look alright to me, though I haven't dug into the error cause.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would this be a case where the nested context managers could help?

Copy link
Contributor

Choose a reason for hiding this comment

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

In some ways, I'd rather we go with that approach so that the error messages are localized and make more sense when contextualized directly above the error-throwing statement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I doubt this one can be nested -- since typing-extensions=4.6.1 only emits one warning rather two, there'd still be a failure when the outer block doesn't throw a warning.

@pydantic-hooky pydantic-hooky bot added awaiting author revision awaiting changes from the PR author and removed ready for review labels Jun 20, 2024
@s-t-e-v-e-n-k s-t-e-v-e-n-k force-pushed the check-all-warnings-with-pytest branch from 3dad616 to 2c7052f Compare June 21, 2024 03:08
@s-t-e-v-e-n-k
Copy link
Contributor Author

I am not sure what is causing the Windows CI failures. 😞

With pytest 8.x, pytest.warns() now emits all warnings, bump the
required version of pytest and then assert that all of them are the
expected warning, rather than filtering them out.

Fixes pydantic#9597
@s-t-e-v-e-n-k s-t-e-v-e-n-k force-pushed the check-all-warnings-with-pytest branch from 2c7052f to 195f8fc Compare June 21, 2024 03:17
@sydney-runkle
Copy link
Contributor

@s-t-e-v-e-n-k,

Ah, looks like the lockfile and pyproject.toml were out of sync.

@sydney-runkle sydney-runkle added relnotes-packaging Used for dependency changes. and removed relnotes-fix Used for bugfixes. labels Jun 21, 2024
Copy link
Contributor

@sydney-runkle sydney-runkle left a comment

Choose a reason for hiding this comment

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

Thanks for your work on this.

I think there's still room to improve the readability of these tests if anyone is interested, though I think this is a good step forward in at least upgrading to pytest v8+

@sydney-runkle sydney-runkle merged commit 09bb639 into pydantic:main Jun 21, 2024
@sydney-runkle
Copy link
Contributor

Here's a link to the pytest warnings docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting author revision awaiting changes from the PR author relnotes-packaging Used for dependency changes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Drop support for pytest 7.4
2 participants