Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Smoke tests for assorted plugins #7721

Merged
merged 18 commits into from
Sep 19, 2020

Conversation

ssbarnea
Copy link
Member

@ssbarnea ssbarnea commented Sep 5, 2020

This aims to create a tox environment that tests that installation of
popular plugins does not break pytest.

Related to pytest-dev/pytest-cov#430

ssbarnea and others added 2 commits September 9, 2020 10:08
This adds a new tox environment that tests that installation of
popular plugins does not break pytest. We can add more as time passes.
@nicoddemus nicoddemus changed the title WIP: Test impact of plugin installation Test impact of plugin installation Sep 9, 2020
@nicoddemus nicoddemus marked this pull request as ready for review September 9, 2020 13:11
@nicoddemus
Copy link
Member

@ssbarnea I've update the PR to include simple integration tests for pytest-cov, pytest-django, pytest-html, and pytest-sugar. I also fixed the issue you identified originally in pytest-cov.

We should backport the commit that adds the integration tests back to 6.0.x after this gets merged.

Fix pytest-dev/pytest-cov#430.

@nicoddemus nicoddemus added the needs backport applied to PRs, indicates that it should be ported to the current bug-fix branch label Sep 12, 2020
Probably that configuration is there for historical reasons, but doesn't
 make much sense to collect any *.py in testing as a potential test nowadays.
@nicoddemus nicoddemus changed the title Test impact of plugin installation Smoke tests for assorted plugins Sep 12, 2020
@nicoddemus
Copy link
Member

I think this is a good starting point, but if others feel some plugin is missing, please speak up!

tox.ini Show resolved Hide resolved
@agronholm
Copy link

You need to ensure that the tests are actually run. Add a failing case with strict xfail=True to make sure. I learned this the hard way when I found out AnyIO's hypothesis support had been silently broken the whole time.

@ssbarnea
Copy link
Member Author

You may want to look at the PYTEST_REQPASS feature added by pytest-plus which assures that an exact number of tests passed. That was aimed for CI/CD usage for avoiding accidents where tests were skipped.

tox.ini Outdated
Comment on lines 139 to 140
pytest pytest_trio_integration.py
pytest pytest_anyio_integration.py
Copy link
Member

Choose a reason for hiding this comment

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

I think we should test all the async frameworks simultaneously - they have all agreed to use marks to allow them to all interoperate in a single test run, so we should test that here

Choose a reason for hiding this comment

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

At least as long as you don't use async fixtures. pytest-asyncio is known to appropriate those regardless of marks (see pytest-dev/pytest-asyncio#124 for the issue).

Copy link
Member

Choose a reason for hiding this comment

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

At least as long as you don't use async fixtures.

That's a very good point, we currently don't test any async fixtures here.

tox.ini Outdated Show resolved Hide resolved
@graingert
Copy link
Member

@ssbarnea what's the scope of this you're happy to accept? I think as plugin maintainers become aware of this PR more will want included.

I don't want to see a situation where this scope creeps all the way to testing all of pypi before it's merged! So I think we should merge this sooner and rather than later so maintainers can add their own smoke tests as PRs directly to pytest, rather than this branch?

@graingert
Copy link
Member

installation of popular plugins does not break pytest.

how popular is popular? Can we nail down a criteria for inclusion?

@altendky
Copy link
Member

You may want to look at the PYTEST_REQPASS feature added by pytest-plus which assures that an exact number of tests passed. That was aimed for CI/CD usage for avoiding accidents where tests were skipped.

Is that better than just requiring coverage in your tests?

Copy link
Member

@altendky altendky left a comment

Choose a reason for hiding this comment

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

The stated purpose is to test that pytest doesn't break. Shouldn't the full pytest suite be run instead of just a trivial case for each plugin?

tox.ini Outdated Show resolved Hide resolved
@ssbarnea
Copy link
Member Author

xdist and cov are ok dor start. good point about avoiding scope creep. we can always add more later.

tox.ini Show resolved Hide resolved
tox.ini Show resolved Hide resolved
@nicoddemus
Copy link
Member

The stated purpose is to test that pytest doesn't break. Shouldn't the full pytest suite be run instead of just a trivial case for each plugin?

This makes sense, but I would rather start things slowly:

  1. Plugins might interact with pytest differently in a way that breaks the pytest suite, but are not wrong: for example pytest-sugar changes output, and several tests check output and would break, requiring us to special case tests, or choosing a subset.
  2. We can always improve things later the more we get the feeling how this will work. Having it being a quick sanity check run is beneficial, and reliable.

I don't want to see a situation where this scope creeps all the way to testing all of pypi before it's merged! So I think we should merge this sooner and rather than later so maintainers can add their own smoke tests as PRs directly to pytest, rather than this branch?

Agreed! This is already a large improvement over what we have today, so let's get this in as soon as we feel this is good enough.

@nicoddemus
Copy link
Member

Add 3 more: pytest-flakes, pytest-qt and pytest-bdd. Also added pip check as suggested.

@nicoddemus
Copy link
Member

Hmm as I feared: pytest-qt, besides depending on a large library, also requires an X server. While it is possible, I don't want to introduce complicated setups at this stage.

@nicoddemus
Copy link
Member

Anything else? If not it would be nice we could get some approvals here and get this merged. 👍

Copy link
Member

@hugovk hugovk left a comment

Choose a reason for hiding this comment

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

This is a great idea! Looks good. I agree on doing light testing of each plugin, this is a quick smoke test* to check nothing exploded. Should catch the major things, and plugins are still encouraged to test against pytest pre-releases.

(*Etymology: 'The term originates in hardware repair and has been applied to software. It's intended to be a quick test to see if the application "catches on fire" when run for the first time.')

@hugovk
Copy link
Member

hugovk commented Sep 19, 2020

Oh, one thing to keep in mind: it's possible a new plugin release will break current master, and a new PR will start to fail because of the plugin release, and not the PR changes.

In such cases, it's a good idea to check if a master build also fails in the same way.

It may be needed to temporarily skip smoke testing that plugin to prevent all PRs failing, until either pytest or the plugin has a fix (because it could be a problem on either side, or both!).

But hopefully this would be rare. And in any case, it's good information to know there's a failure.

Copy link
Member

@bluetech bluetech left a comment

Choose a reason for hiding this comment

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

As the person doing most of the breaking, this will be very helpful!

@nicoddemus
Copy link
Member

Thanks everyone!

@nicoddemus
Copy link
Member

Of course everyone feel free to add more plugins that they consider relevant. 👍

@nicoddemus nicoddemus merged commit b031a7c into pytest-dev:master Sep 19, 2020
nicoddemus added a commit to nicoddemus/pytest that referenced this pull request Sep 19, 2020
Co-authored-by: Bruno Oliveira <[email protected]>
Co-authored-by: Thomas Grainger <[email protected]>
Co-authored-by: Kyle Altendorf <[email protected]>
nicoddemus added a commit that referenced this pull request Sep 19, 2020
[6.0.x] Smoke tests for assorted plugins (#7721)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs backport applied to PRs, indicates that it should be ported to the current bug-fix branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants