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

Update pytest requirement from ~=7.4 to ~=8.2 #9576

Merged
merged 9 commits into from
Jun 4, 2024

Conversation

dependabot[bot]
Copy link
Contributor

@dependabot dependabot bot commented on behalf of github Apr 29, 2024

Updates the requirements on pytest to permit the latest version.

Release notes

Sourced from pytest's releases.

8.2.0

pytest 8.2.0 (2024-04-27)

Deprecations

  • #12069: A deprecation warning is now raised when implementations of one of the following hooks request a deprecated py.path.local parameter instead of the pathlib.Path parameter which replaced it:

    • pytest_ignore_collect{.interpreted-text role="hook"} - the path parameter - use collection_path instead.
    • pytest_collect_file{.interpreted-text role="hook"} - the path parameter - use file_path instead.
    • pytest_pycollect_makemodule{.interpreted-text role="hook"} - the path parameter - use module_path instead.
    • pytest_report_header{.interpreted-text role="hook"} - the startdir parameter - use start_path instead.
    • pytest_report_collectionfinish{.interpreted-text role="hook"} - the startdir parameter - use start_path instead.

    The replacement parameters are available since pytest 7.0.0. The old parameters will be removed in pytest 9.0.0.

    See legacy-path-hooks-deprecated{.interpreted-text role="ref"} for more details.

Features

  • #11871: Added support for reading command line arguments from a file using the prefix character @, like e.g.: pytest @tests.txt. The file must have one argument per line.

    See Read arguments from file <args-from-file>{.interpreted-text role="ref"} for details.

Improvements

  • #11523: pytest.importorskip{.interpreted-text role="func"} will now issue a warning if the module could be found, but raised ImportError{.interpreted-text role="class"} instead of ModuleNotFoundError{.interpreted-text role="class"}.

    The warning can be suppressed by passing exc_type=ImportError to pytest.importorskip{.interpreted-text role="func"}.

    See import-or-skip-import-error{.interpreted-text role="ref"} for details.

  • #11728: For unittest-based tests, exceptions during class cleanup (as raised by functions registered with TestCase.addClassCleanup <unittest.TestCase.addClassCleanup>{.interpreted-text role="meth"}) are now reported instead of silently failing.

  • #11777: Text is no longer truncated in the short test summary info section when -vv is given.

  • #12112: Improved namespace packages detection when consider_namespace_packages{.interpreted-text role="confval"} is enabled, covering more situations (like editable installs).

  • #9502: Added PYTEST_VERSION{.interpreted-text role="envvar"} environment variable which is defined at the start of the pytest session and undefined afterwards. It contains the value of pytest.__version__, and among other things can be used to easily check if code is running from within a pytest run.

Bug Fixes

  • #12065: Fixed a regression in pytest 8.0.0 where test classes containing setup_method and tests using @staticmethod or @classmethod would crash with AttributeError: 'NoneType' object has no attribute 'setup_method'.

    Now the request.instance <pytest.FixtureRequest.instance>{.interpreted-text role="attr"} attribute of tests using @staticmethod and @classmethod is no longer None, but a fresh instance of the class, like in non-static methods.

... (truncated)

Commits
  • 6bd3f31 Tweak changelog for 8.2.0
  • 9b6219b Prepare release version 8.2.0
  • 835765c Merge pull request #12130 from bluetech/fixtures-inline
  • 7e7503c unittest: report class cleanup exceptions (#12250)
  • 882c4da fixtures: inline fail_fixturefunc
  • 2e8fb9f fixtures: extract a _check_fixturedef method
  • acf2971 fixtures: inline _getnextfixturedef into _get_active_fixturedef
  • 3c77aec fixtures: move "request" check early
  • d217d68 fixtures: inline _compute_fixture_value
  • 530be28 fixtures: use early return in _get_active_fixturedef
  • Additional commits viewable in compare view

You can trigger a rebase of this PR by commenting @dependabot rebase.


Dependabot commands and options

You can trigger Dependabot actions by commenting on this PR:

  • @dependabot rebase will rebase this PR
  • @dependabot recreate will recreate this PR, overwriting any edits that have been made to it
  • @dependabot merge will merge this PR after your CI passes on it
  • @dependabot squash and merge will squash and merge this PR after your CI passes on it
  • @dependabot cancel merge will cancel a previously requested merge and block automerging
  • @dependabot reopen will reopen this PR if it is closed
  • @dependabot close will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually
  • @dependabot show <dependency name> ignore conditions will show all of the ignore conditions of the specified dependency
  • @dependabot ignore this major version will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)
  • @dependabot ignore this minor version will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)
  • @dependabot ignore this dependency will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)

Note
Automatic rebases have been disabled on this pull request as it has been open for over 30 days.

@dependabot dependabot bot added dependency Label for github dependabot Skip news 🔇 This change does not require a changelog entry labels Apr 29, 2024

This comment has been minimized.

@Pierre-Sassoulas Pierre-Sassoulas added this to the 3.2.0 milestone Apr 29, 2024
@Pierre-Sassoulas Pierre-Sassoulas added the Blocker 🙅 Blocks the next release label Apr 29, 2024
@Pierre-Sassoulas
Copy link
Member

@dependabot recreate

@dependabot dependabot bot force-pushed the dependabot/pip/pytest-approx-eq-8.2 branch from 3771d98 to 3812d0d Compare April 29, 2024 21:49

This comment has been minimized.

@Pierre-Sassoulas
Copy link
Member

Pierre-Sassoulas commented May 1, 2024

I bisected this issue to pytest-dev/pytest@a21fb8 (introduced in pytest-dev/pytest#11138 released in pytest 8.0.0), this was rather long to do because launching a failing test alone does not trigger the error (You have to launch the whole test suite to reproduce). I supposed there's some test contamination in the pylint test suite initially but it might also be an issue with the test discovery from pytest seeing the offending commit.

@Pierre-Sassoulas Pierre-Sassoulas added High effort 🏋 Difficult solution or problem to solve Needs investigation 🔬 A bug or crash where it's not immediately obvious what is happenning and removed Blocker 🙅 Blocks the next release labels May 1, 2024
@Pierre-Sassoulas
Copy link
Member

Removing blocker label because we can release by testing with pytest < 8.0.0

@Pierre-Sassoulas Pierre-Sassoulas removed this from the 3.2.0 milestone May 1, 2024
@Pierre-Sassoulas Pierre-Sassoulas dismissed their stale review May 1, 2024 12:43

Need to investigate and fix a test contamination or worse

@Pierre-Sassoulas Pierre-Sassoulas force-pushed the dependabot/pip/pytest-approx-eq-8.2 branch from 4cf3b21 to 49a7a61 Compare May 1, 2024 15:54

This comment has been minimized.

Copy link
Contributor Author

dependabot bot commented on behalf of github May 20, 2024

A newer version of pytest exists, but since this PR has been edited by someone other than Dependabot I haven't updated it. You'll get a PR for the updated version as normal once this PR is merged.

dependabot bot and others added 2 commits June 3, 2024 23:09
Updates the requirements on [pytest](https://github.com/pytest-dev/pytest) to permit the latest version.
- [Release notes](https://github.com/pytest-dev/pytest/releases)
- [Changelog](https://github.com/pytest-dev/pytest/blob/main/CHANGELOG.rst)
- [Commits](pytest-dev/pytest@7.4.0...8.2.0)

---
updated-dependencies:
- dependency-name: pytest
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <support@github.com>
Suggestion from pytest-dev/pytest#11138 (comment)

With 'pytest' (launching the whole pylint test suite):
FAILED tests/test_precedence.py::test_package - AssertionError: E: 21: Module 'package.AudioTime' has no 'DECIMAL' member<function Equals.<locals>.<lambda> at 0x76c566741750>

With 'pytest tests/test_precedence.py':
tests/test_precedence.py .                                                                                                                                                                           [100%]

============================================================================================ 1 passed in 1.04s =============================================================================================

With 'python tests/test_precedence.py':
Checked ['package.__init__'] successfully
Checked ['precedence_test'] successfully
Checked ['import_package_subpackage_module'] successfully
Checked ['pylint.checkers.__init__'] successfully
Checked ['/home/pierre/pylint/tests/regrtest_data/classdoc_usage.py'] successfully
Checked ['/home/pierre/pylint/tests/regrtest_data/module_global.py'] successfully
Checked ['/home/pierre/pylint/tests/regrtest_data/decimal_inference.py'] successfully
Checked ['/home/pierre/pylint/tests/regrtest_data/absimp/string.py'] successfully
Checked ['/home/pierre/pylint/tests/regrtest_data/bad_package'] successfully
@DanielNoord DanielNoord force-pushed the dependabot/pip/pytest-approx-eq-8.2 branch from 604c55d to 80cd3a3 Compare June 3, 2024 21:11
@DanielNoord DanielNoord force-pushed the dependabot/pip/pytest-approx-eq-8.2 branch from 80cd3a3 to bf334db Compare June 3, 2024 21:23
@Pierre-Sassoulas
Copy link
Member

Damn, nice catch !

Who is to blame for this horrendous foo collision ? Let's hang the culprit in public place !

disable-disallowed-name

https://github.com/pytest-dev/pytest/blame/7be95f9b30ffd418483bdb57112f9339465d4695/pyproject.toml#L196

Ho, no.

This comment has been minimized.

@DanielNoord
Copy link
Collaborator

Used https://github.com/asottile/detect-test-pollution which is really helpful here.

Found another pollution. The mock of sys.exit was actually being inferred in the functional tests. I could reproduce that down to 7.3.0 so I guess it was just pytest being faster on CI that now exposes this issue? I don't know man..

This comment has been minimized.

@DanielNoord DanielNoord removed the Needs investigation 🔬 A bug or crash where it's not immediately obvious what is happenning label Jun 3, 2024
@DanielNoord
Copy link
Collaborator

DanielNoord commented Jun 3, 2024

I think that is the last of it.

I really don't understand why the bump causes this. My main intuition is just faster test runs make it so the pollution is more prominent but I have no idea. All of these issues existed before this PR as well.

Edit: Looking at the output it is pretty clear. 8.x changes the ordering of the tests. Instead of doing all files in root first and then iterating through directories it just does them all alphabetically, without differentiating between a directory or a file.

@Pierre-Sassoulas I think this is ready for review now.

Copy link

codecov bot commented Jun 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.83%. Comparing base (4203d87) to head (f495fb5).
Report is 146 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #9576      +/-   ##
==========================================
+ Coverage   95.82%   95.83%   +0.01%     
==========================================
  Files         174      174              
  Lines       18810    18809       -1     
==========================================
+ Hits        18024    18025       +1     
+ Misses        786      784       -2     
Files with missing lines Coverage Δ
pylint/testutils/configuration_test.py 100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

This comment has been minimized.

Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

Thank you, amazing !

So the precise commit causing the issue is pytest-dev/pytest@a21fb8, looking at it, I don't think it was a speed issue. My intuition is now that it's:

The path property of Package nodes now points to the package directory instead of the __init__.py file.

that changed the way the pyi were taken into account (?). The ast is also modified in pytest see pytest-dev/pytest#11138 (comment) (which is an issue really, maybe pylint doesn't actually work without what pytest does to the AST first... but I don't really want to migrate back to unittest !) so it could be something else entirely (?)

All in all, very hard to tell without even more bisection inside this particular commit and analysis. As you managed to bruteforce our way to a green CI we don't need to understand how we could have done it super fast by understanding what happened 😅 . But it does bug me. And pylint-pytest have similar issues with pytest 8.2.1 (see pylint-dev/pylint-pytest#67 (comment)) so understanding what happened could still help.

@Pierre-Sassoulas Pierre-Sassoulas enabled auto-merge (squash) June 4, 2024 06:32
@DanielNoord
Copy link
Collaborator

So the precise commit causing the issue is pytest-dev/pytest@a21fb8,

Ah of course!

that changed the way the pyi were taken into account (?).

No, it just changes the test execution order. All these failures could be reproduced on older versions of pytest. I checked that to rule out what was going on. It was purely a test pollution issue that we never found earlier.

But it does bug me. And pylint-pytest have similar issues with pytest 8.2.1 (see pylint-dev/pylint-pytest#67 (comment)) so understanding what happened could still help.

With https://github.com/asottile/detect-test-pollution it really is easy to find the pollution. Takes about 2 or 3 times the total execution time of the full suite.

@Pierre-Sassoulas Pierre-Sassoulas merged commit ba33325 into main Jun 4, 2024
44 checks passed
@Pierre-Sassoulas Pierre-Sassoulas deleted the dependabot/pip/pytest-approx-eq-8.2 branch June 4, 2024 06:40
Copy link
Contributor

github-actions bot commented Jun 4, 2024

🤖 According to the primer, this change has no effect on the checked open source code. 🤖🎉

This comment was generated for commit f495fb5

@Pierre-Sassoulas
Copy link
Member

Hmm, right, I should just have searched for test contamination given it was my first hypothesis (pytest-dev/pytest#11138 (comment)) before searching for more convoluted issues 😅 Thanks for handling it !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependency Label for github dependabot High effort 🏋 Difficult solution or problem to solve Skip news 🔇 This change does not require a changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants