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 May 30, 2024

Change Summary

Building on Michał's amazing work in #9040 , extend the pattern to also ignore other warnings that are emitted by pytest.warns() when running under Pytest 8.x. This also continues to support Pytest 7.x.

Co-authored-by: Michał Górny mgorny@gentoo.org

Related issue number

Fixes: #9025

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: @hramezani

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

please review

Copy link

codspeed-hq bot commented May 30, 2024

CodSpeed Performance Report

Merging #9527 will not alter performance

Comparing s-t-e-v-e-n-k:ignore-other-warnings-pytest-8 (ed71c11) with main (8b42832)

Summary

✅ 13 untouched benchmarks

@s-t-e-v-e-n-k s-t-e-v-e-n-k force-pushed the ignore-other-warnings-pytest-8 branch from a78313b to 266bac9 Compare May 30, 2024 04:24
Building on Michał's amazing work in PR#9040, extend the pattern to also
ignore other warnings that are emitted by pytest.warns() when running
under Pytest 8.x. This also continues to support Pytest 7.x.

Co-authored-by: Michał Górny <mgorny@gentoo.org>

Fixes: pydantic#9025
@s-t-e-v-e-n-k s-t-e-v-e-n-k force-pushed the ignore-other-warnings-pytest-8 branch from 266bac9 to 7fb3152 Compare May 30, 2024 04:32
@sydney-runkle
Copy link
Contributor

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

Thanks for your work here! Is the idea that both this and #9040 will need to be merged to fix #9025?

Happy to review more thoroughly, just wanted to better understand the context of this PR :).

os.environ['PYDANTIC_ERRORS_OMIT_URL'] = 'true'
os.environ['PYDANTIC_ERRORS_INCLUDE_URL'] = 'false'
Copy link
Contributor

Choose a reason for hiding this comment

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

This could potentially be a breaking change (though I'm not sure who might have this enabled?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could it, in a conftest? Based on the commit message for that bit in #9040 this also to silence a warning.

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

@sydney-runkle Thanks for the linting fix, but no, this PR can be merged as-is -- it builds on the work done in #9040 so I wanted to make certain @mgorny was also credited for the work done.

@sydney-runkle
Copy link
Contributor

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

Thanks for the awesome work here. One lingering question that I still have - for the case where we're catching multiple warnings, what about something like:

import warnings
import pytest

def test_multiple_warnings():
    with pytest.warns((UserWarning, RuntimeWarning)) as record:
        warnings.warn("This is a user warning", UserWarning)
        warnings.warn("This is a runtime warning", RuntimeWarning)

    assert len(record) == 2  # check that two warnings were raised
    assert isinstance(record[0].message, UserWarning)  # check the type of the first warning
    assert str(record[0].message) == "This is a user warning"  # check the first warning message
    assert isinstance(record[1].message, RuntimeWarning)  # check the type of the second warning
    assert str(record[1].message) == "This is a runtime warning"  # check the second warning message

The only thing that I feel is missing from the current implementation is details regarding the second, third, etc warnings being raised, which I think we want to preserve.

@sydney-runkle sydney-runkle added awaiting author revision awaiting changes from the PR author and removed ready for review labels Jun 6, 2024
@s-t-e-v-e-n-k
Copy link
Contributor Author

I completely agree that would be an excellent change, and also allows us to know which code paths raise what warnings -- however, that change will also drop support for pytest 7.4 -- I'm happy to do so, but it would be a larger change.

@sydney-runkle
Copy link
Contributor

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

Ok, gotcha. This looks good then. I appreciate the TODO additions to have as a future reference!

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.

Looks good, thanks for your work on this folks.

When do you guys think it makes sense to drop support for Pytest 7.4?

@sydney-runkle sydney-runkle merged commit 0c180c8 into pydantic:main Jun 7, 2024
@mgorny
Copy link
Contributor

mgorny commented Jun 7, 2024

Thanks!

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-fix Used for bugfixes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

-Werror-related test failures with pytest 8.x
4 participants