Skip to content

Conversation

gnikonorov
Copy link
Member

In the tests, check to make sure that:

  • The correct warnings are reported
  • The warnings text is correct

Before, we weren't checking but I think we should be since we are setting these warnings in this plugin, and they are not coming from pytest.

No change log as this is trivial

@gnikonorov gnikonorov requested a review from icemac July 19, 2020 19:56
@gnikonorov gnikonorov self-assigned this Jul 19, 2020
@icemac
Copy link
Contributor

icemac commented Jul 20, 2020

Hm, the tests fail for pytest < 5.3. Do you have any idea why?

@The-Compiler
Copy link
Member

The problem is that the stored outcomes are {'failed': 1, 'rerun': 3, 'warnings': 1} but you're checking for 'warning' (singular). There were various changes to the plurality there, starting with pytest-dev/pytest#5990 - I'm not sure if warnings works on all versions though... Probably worth a try?

@gnikonorov
Copy link
Member Author

The problem is that the stored outcomes are {'failed': 1, 'rerun': 3, 'warnings': 1} but you're checking for 'warning' (singular). There were various changes to the plurality there, starting with pytest-dev/pytest#5990 - I'm not sure if warnings works on all versions though... Probably worth a try?

Thanks for the tip @The-Compiler, let me try tonight

@gnikonorov gnikonorov changed the title Check for warnings and their output during unit tests Check for warnings output during unit tests Jul 21, 2020
@gnikonorov
Copy link
Member Author

So I realized what I was doing was inherently wrong. Checking general warnings is extremely fragile at best, since pytest itself emits warnings and there's no way of knowing which warnings came from this plugin vs. pytest. Checking to make sure we emitted at least some amount of warnings 'x' also doesn't tell us anything since those warnings could be coming from somewhere else, so the test would give a false sense of security or just confuse people.

That leads me to think there are 2 possible solutions:

  • We keep track of the number of warnings emitted by this plugin in the plugin itself, and compare against that. I personally don't think this is the best idea since we're adding code purely for test purposes, and there's no guarantee that people will maintain this convention
  • We do what I updated this PR to do, and just check that stdout contains the warning text we want. I think this is best, since it makes no assumption on the warnings pytest emits, and requires no special maintenance.

Thoughts @icemac and @The-Compiler ?

Copy link
Contributor

@icemac icemac left a comment

Choose a reason for hiding this comment

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

LGTM. I think we can go with this approach.

@@ -1,4 +1,5 @@
from unittest import mock
import pkg_resources
Copy link
Contributor

Choose a reason for hiding this comment

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

This import is no longer needed.

Copy link
Member

@The-Compiler The-Compiler left a comment

Choose a reason for hiding this comment

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

Looking at the warning text sounds like a good idea. I commented on a minor change, but LGTM otherwise!

result = testdir.runpytest('--reruns', '3',
'--reruns-delay', str(delay_time))

num_warnings = 0
Copy link
Member

Choose a reason for hiding this comment

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

This could probably be a has_warnings or warning_expected boolean now?

Copy link
Member Author

Choose a reason for hiding this comment

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

ah yes, this is wrong. sorry for leaving it, let me remove it

Copy link
Member

Choose a reason for hiding this comment

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

The new version looks better, though now you have two identical if statements after each other.

Copy link
Member Author

Choose a reason for hiding this comment

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

sorry just changed it

Copy link
Member

@sallner sallner left a comment

Choose a reason for hiding this comment

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

👍 This can be merged after rebase.

@gnikonorov gnikonorov merged commit be9f031 into pytest-dev:master Jul 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

4 participants