WarningsRecorder.pop() improperly matches warning

[groutr]

When trying to pop a specific warning from a WarningsRecorder instance, the wrong warning is returned. I believe the issue is that pop uses issubclass

pytest/src/_pytest/recwarn.py

Line 210 in 3c15349

if issubclass(w.category, cls):

I believe the correct comparison should be:

if w.category is cls:

Here is a minimum working example that triggers the buggy behavior:

import pytest
import warnings

class RWarning(Warning):
    pass
    
class SWarning(RWarning):
    pass

def raise_warnings():
    warnings.warn("Warning 1", SWarning)
    warnings.warn("Warning 2", RWarning)
    
def test_pop():
    with pytest.warns((RWarning, SWarning)) as record:
        raise_warnings()
        
    assert len(record) == 2
    _warn = record.pop(RWarning)
    assert _warn.category is RWarning  # This fails because _warn.category is SWarning

The test output is

========================================================================================= FAILURES ==========================================================================================
_________________________________________________________________________________________ test_pop __________________________________________________________________________________________

    def test_pop():
        with pytest.warns((RWarning, SWarning)) as record:
            raise_warnings()

        assert len(record) == 2
        _warn = record.pop(RWarning)
>       assert _warn.category is RWarning
E       AssertionError: assert <class 'pytest_bug.SWarning'> is RWarning
E        +  where <class 'pytest_bug.SWarning'> = <warnings.WarningMessage object at 0x7fea08a72010>.category

pytest_bug.py:24: AssertionError

pytest 7.2.1 on archlinux.
virtual environment is a clean conda environment with only python and pytest (and their dependencies installed from conda-forge).

If this is indeed a bug, I'm happy to open a PR with my proposed solution.

[Zac-HD]

Yeah, it looks like this code never anticipated the possibility that a later warning might be a better match than the current warning. That said, we need to preserve the current behavior of matching if the warning was a subclass, so instead of the current

for i, w in enumerate(self._list):
    if issubclass(w.category, cls):
        return self._list.pop(i)

I propose we do something like

# Match the first warning that is a subtype of `cls`, where there
# is *not* a later captured warning which is a subtype of this one.
matches = []
for i, w in enumerate(self._list):
    if w.category == cls:  # in this case we can exit early
        return self._list.pop(i)
    if issubclass(w.category, cls):
        matches.append((i, w))
if not matches:
    raise ... # see current code for details
(idx, best), *rest = matches
for i, w in rest:
    if issubclass(w, best) and not issubclass(best, w):
        idx, best = i, w
return self._list.pop(idx)

and it would be awesome if you could open a PR with this, tests, a changelog entry, and of course adding yourself to the contributors list 🙏

[groutr]

Thank you for your comments @Zac-HD. Let me start by describing the challenge I am trying to work out in my code. I have a function that returns several warnings, and I want to check that for a certain set of arguments, all the expected warnings are emitted. This would mean checking that the proper warning classes have been captured and a matched portion of the warning message is also present. In an ideal world, I would think my test would probably look like:

def test_warnings():
    with pytest.warns(RWarning) as record:   # RWarning is the base warning class for the library and all other warning inherit from it
        my_function('a', 'b')
    assert len(record) == 2
    assert record.contains(RWarning, match_expr="Warning2") # this would be looking explicitly for RWarning, not its subclasses
    assert record.contains(SWarning, match_expr="Warning1")

I was thinking about this more and testing the code you wrote and feel that maybe a separate method would be a better solution.

My ideas are:

1. a .contains method that does exact class checking. True if specified warning exists (and if an optional match_expr matches the warning message). See above for possible usage example. Returns True/False
   Possible implementation

def contains(self, cls, match_expr=None):
    if match_expr:
        match_re = re.compile(match_expr)
        for w in self._list:
            if w.category is cls and match_re.search(str(w.message)):
                return True
    else:
        for w in self._list:
            if w.category is cls:
                return True
    return False

2. a .search or .findall (inspired by the regex library) method that can do strict or subclass checking with a keyword argument switch. Returns a list of matching warnings. This could also do message matching, if desired.

m = list(record.match(RWarning, subclasses=False)) # do exact class matching
m = list(record.match(RWarning, subclasses=True)) # match with classes that are RWarnings or where RWarning is a base class

Possible implentation:

def match(self, cls, subclasses=False):
    """ Match cls or cls subclasses """
    if subclasses:
        op = issubclass
    else:
        op = operator.is_
	
    for i, w in enumerate(self._list):
	if op(w.category, cls):
            yield i

# Then .pop becomes
def pop(self, cls):
    try:
        i = next(self.match(cls, subclasses=True))
        return self._list.pop(i)
    except StopIteration:
        __tracebackhide__ = True
        raise AssertionError(f"{cls!r} not found in warning list")

What are your thoughts, @Zac-HD?

[Zac-HD]

Seems like you might be better of with the stdlib with warnings.catch_warnings(record=True, ...) as list_of_caught_warnings: function, and making assertions manually?

Pytest helpers are nice, but sometimes it's worth using the rest of Python instead 😁

[RonnyPfannschmidt]

It does remind me of the plan to integrate dirty equals better into pytest, it would be nice if there was a way to spell the warning matches for the contains check

[lesnek]

Hello @Zac-HD , I want to start helping with open source, and I can see it is a good first issue.
Do you know if I can take it? I started from your proposal, added the example to tests, and worked out this solution on my fork of pytest. Did I understand the fix, and is it acceptable to be reviewed as a pull request?