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

Add --version and --pytest-plugins options to Pytest and deprecate old options #8619

Merged
merged 1 commit into from
Nov 19, 2019

Conversation

Eric-Arellano
Copy link
Contributor

@Eric-Arellano Eric-Arellano commented Nov 13, 2019

Problem

The options config for the V1 Pytest subsystem is not ideal. Rather than a specific option for each plugin like --unittest2-requirements, we should have a generic --plugins option that is a list type so that users can easily add or remove plugins.

We need this to add the plugin pytest-rerunfailures.

Solution

Add new options --version and --pytest-plugins to the Pytest subsystem. Deprecate the old options.

Set up the parsing logic as follows:

  • it's invalid to specify both the new and old style
  • if the old style was manually configured, entirely use the old style
  • otherwise, use the new style

Result

It's now much easier to add and remove plugins to the subsystem.

If the user uses the old style, we print:

[WARN] /Users/eric/DocsLocal/code/projects/pants/src/python/pants/bin/pants_loader.py:85: DeprecationWarning: DEPRECATED: option 'requirements' in scope 'pytest' will be removed in version 1.25.0.dev0.
Use --version instead.

If the user specifies both the old and new style, we print:

Exception message: Conflicting options for --pytest used. You provided these options in the new, preferred style: --pytest-version, but also provided these options in the deprecated style: --pytest-requirements, --pytest-timeout-requirements.
Please use only one approach (preferably the new approach of --version and --pytest-plugins).

super().register_options(register)
register('--version', default='pytest>=5.2.2', help="Requirement string for Pytest.")
register(
'--pytest-plugins',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

NB: this, unfortunately, cannot be plugins because that is taken by the global namespace :/

@stuhood
Copy link
Member

stuhood commented Nov 13, 2019

It would definitely be preferable to use the existing subsystem, and/or to remove our existing plugins if it looks like they are not necessary (are they not necessary for v2?)

We need to use a more modern Pytest version than <3.7.0 (current release is 5.2.2).

Why is that?

@Eric-Arellano
Copy link
Contributor Author

We need to use a more modern Pytest version than <3.7.0 (current release is 5.2.2).

Why is that?

pytest-rerunfailures requires >=4.4 and Toolchain needs more modern Pytest too.

It would definitely be preferable to use the existing subsystem, and/or to remove our existing plugins if it looks like they are not necessary (are they not necessary for v2?)

I agree. I'm trying to land #8621 to then be able to upgrade Pytest and make the options changes for the original Pytest system. This has the added benefit of allowing us to use the pytest-rerunfailures plugin for our V1 test runner too. Last year, John ran into quite confusing Rust memory issues with #8621 though, so fingers crossed those don't happen anymore.

@benjyw
Copy link
Contributor

benjyw commented Nov 14, 2019

It would definitely be preferable to use the existing subsystem, and/or to remove our existing plugins if it looks like they are not necessary (are they not necessary for v2?)

Using the existing subsystem is tricky, because we can't get newer pytests to work with v1, and I don't think it's worth the effort to fix that. Also, as Eric mentions, the existing subsystem options are poorly designed, and working around that requires deprecation and extra logic. At this point it seems better to throw all our energy into v2 and get off the v1 python pipeline ASAP.

@Eric-Arellano Eric-Arellano changed the title Add V2 subsystem with modern Pytest and plugins options Add --version and --pytest-plugins options to Pytest and deprecate old options Nov 18, 2019
@Eric-Arellano Eric-Arellano force-pushed the v2-pytest-subsystem branch 2 times, most recently from 9cdc747 to bcb1530 Compare November 18, 2019 20:46
@Eric-Arellano
Copy link
Contributor Author

Now that we know we'll be able to get the V1 test runner working with more modern Pytest (thanks to #6320), I changed this PR so that all it does is add the new options and deprecate the old.

In a followup PR, I will upgrade the subsystem to use the newest versions of Pytest, which will allow us to use the retry plugin.

register('--timeout-requirements', advanced=True, default='pytest-timeout>=1.2,<1.3',
help='Requirements string for the pytest-timeout library.')
help='Requirements string for the pytest-timeout library.',
removal_version="1.25.0.dev0", removal_hint="Use --pytest-plugins instead.")
Copy link
Member

Choose a reason for hiding this comment

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

Is there a good reason not to just have a single monolithic list to put all requirements in? It looks like we're just merging them anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Benjy and I talked about that. I argued for having --version be distinct from --pytest-plugins due to the ergonomics of modifying a list. Let's say a user likes our default plugins but wants a newer version of Pytest. They couldn't easily swap out the old Pytest version for the new one, so they'd have to end up doing this:

[pytest]
requirements: ['pytest>5.7', 'pytest-cov>=2.4,<2.5', 'pytest-cov>=2.4,<2.5']

Doing this would mean no more plugins:

[pytest]
requirements: ['pytest>5.7']

Instead, we allow for this:

[pytest]
version: "pytest>5.7"
pytest_plugins: +["pytest-flaky"]

--

Finally, what would we name a unified list? requirements is already sadly taken.

@Eric-Arellano Eric-Arellano merged commit f9384e0 into pantsbuild:master Nov 19, 2019
@Eric-Arellano Eric-Arellano deleted the v2-pytest-subsystem branch November 19, 2019 01:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants