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.

Sign up for GitHub

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

Jump to bottom

Make the order of functional tests deterministic #7602

Merged
merged 1 commit into from
Oct 24, 2022
Merged

Make the order of functional tests deterministic #7602

merged 1 commit into from
Oct 24, 2022

Conversation

kriek
Copy link
Contributor

@kriek kriek commented Oct 10, 2022 •
edited
Loading

Type of Changes

Type
✓ Tests

Description

The pull request fixes the non deterministic ordering of functional tests depending on the underlying system.

Refs #7464

@kriek
Copy link
Contributor Author

kriek commented Oct 13, 2022

@Pierre-Sassoulas I've analyzed the failure with an interesting finding: we miss determinism in functional tests order. On Windows, I have no_member_type_introspection before no_member_type_introspection_base while on Linux, the order seems the other way around. Because we are dealing with a cache bug, the order affects the test outcome.

I propose to remove that non-determinism in get_functional_test_files_from_directory.

@Pierre-Sassoulas
Copy link
Member

I'm wondering if it's possible to fix the cache issue itself instead ?

@kriek
Copy link
Contributor Author

kriek commented Oct 13, 2022

The cache issue is on the Astroid side. My recommandation would be to do it in 3 steps:

  1. Add the test (this PR)
  2. Fix the bug (Astroid side, thru dedicated issue)
  3. Update the test when Astroid requirement is updated in pytest repo to a version shipping the fix

Does it look good to you?

@DanielNoord
Copy link
Collaborator

Are we sure that pylint is the right place for tests? The astroid test suite is much smaller currently so I'm always in favour of putting tests there if they actually belong there.

@coveralls
Copy link

coveralls commented Oct 13, 2022 •
edited
Loading

Pull Request Test Coverage Report for Build 3267696571

  • 3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.0005%) to 95.349%
Totals Coverage Status
Change from base Build 3265567961: 0.0005%
Covered Lines: 17139
Relevant Lines: 17975
💛 - Coveralls

@github-actions

This comment has been minimized.

Sign in to view
@kriek
Copy link
Contributor Author

kriek commented Oct 13, 2022

I agree the test is more relevant on the Astroid side. But do we have to choose? Why not both?

@DanielNoord
Copy link
Collaborator

I agree the test is more relevant on the Astroid side. But do we have to choose? Why not both?

CI takes about 6 minutes now, about 4 on my own machine. That's quite long if you just want to fix something and see if tests pass. Why test something that is not part of the library you're working on? Should we start testing the stdlib as well? It also gives a false sense of security as even if we regress there is nothing to do in this library to fix that regression.

@kriek
Copy link
Contributor Author

kriek commented Oct 13, 2022

Hum, I thought those new tests had a bit of value because they are functional pylint tests about something with unclear outcome at first (i.e. is pylint able to follow type(self)?).

But I see your point and when I think more about it, it is indeed fully out of pylint control: if astroid is able to correctly infer type(self), the relevant no-member findings will follow and any existing no-member functional test should cover the part under pylint responsibility already. I agree to abandon the proposed new pylint tests.

Should we keep the commit to make the order of functional tests deterministic whatever the system?

@DanielNoord
Copy link
Collaborator

Should we keep the commit to make the order of functional tests deterministic whatever the system?

Yes, that's a nice fix!

@Pierre-Sassoulas Pierre-Sassoulas added Skip news 🔇 This change does not require a changelog entry and removed Work in progress labels Oct 17, 2022
@github-actions
Copy link
Contributor

🤖 Effect of this PR on checked open source code: 🤖

Effect on pandas:
The following messages are no longer emitted:

  1. comparison-with-callable:
    Comparing against a callable, did you omit the parenthesis?
    https://github.com/pandas-dev/pandas/blob/7ebc3e809491cdec3ffe4dc8917ad9729f55b713/pandas/tests/series/indexing/test_delitem.py#L72
  2. comparison-with-callable:
    Comparing against a callable, did you omit the parenthesis?
    https://github.com/pandas-dev/pandas/blob/7ebc3e809491cdec3ffe4dc8917ad9729f55b713/pandas/tests/frame/test_constructors.py#L903
  3. comparison-with-callable:
    Comparing against a callable, did you omit the parenthesis?
    https://github.com/pandas-dev/pandas/blob/7ebc3e809491cdec3ffe4dc8917ad9729f55b713/pandas/tests/frame/test_constructors.py#L904
  4. comparison-with-callable:
    Comparing against a callable, did you omit the parenthesis?
    https://github.com/pandas-dev/pandas/blob/7ebc3e809491cdec3ffe4dc8917ad9729f55b713/pandas/tests/frame/test_constructors.py#L908
  5. comparison-with-callable:
    Comparing against a callable, did you omit the parenthesis?
    https://github.com/pandas-dev/pandas/blob/7ebc3e809491cdec3ffe4dc8917ad9729f55b713/pandas/tests/frame/test_constructors.py#L909
  6. comparison-with-callable:
    Comparing against a callable, did you omit the parenthesis?
    https://github.com/pandas-dev/pandas/blob/7ebc3e809491cdec3ffe4dc8917ad9729f55b713/pandas/tests/frame/test_constructors.py#L2674
  7. comparison-with-callable:
    Comparing against a callable, did you omit the parenthesis?
    https://github.com/pandas-dev/pandas/blob/7ebc3e809491cdec3ffe4dc8917ad9729f55b713/pandas/tests/frame/methods/test_combine_first.py#L343
  8. comparison-with-callable:
    Comparing against a callable, did you omit the parenthesis?
    https://github.com/pandas-dev/pandas/blob/7ebc3e809491cdec3ffe4dc8917ad9729f55b713/pandas/tests/dtypes/test_dtypes.py#L272
  9. no-member:
    Instance of 'DatetimeIndex' has no 'round' member
    https://github.com/pandas-dev/pandas/blob/7ebc3e809491cdec3ffe4dc8917ad9729f55b713/pandas/tests/arrays/test_datetimelike.py#L653
  10. comparison-with-callable:
    Comparing against a callable, did you omit the parenthesis?
    https://github.com/pandas-dev/pandas/blob/7ebc3e809491cdec3ffe4dc8917ad9729f55b713/pandas/tests/indexes/period/test_constructors.py#L211
  11. comparison-with-callable:
    Comparing against a callable, did you omit the parenthesis?
    https://github.com/pandas-dev/pandas/blob/7ebc3e809491cdec3ffe4dc8917ad9729f55b713/pandas/tests/indexes/period/test_constructors.py#L216
  12. comparison-with-callable:
    Comparing against a callable, did you omit the parenthesis?
    https://github.com/pandas-dev/pandas/blob/7ebc3e809491cdec3ffe4dc8917ad9729f55b713/pandas/tests/indexes/period/test_constructors.py#L225
  13. comparison-with-callable:
    Comparing against a callable, did you omit the parenthesis?
    https://github.com/pandas-dev/pandas/blob/7ebc3e809491cdec3ffe4dc8917ad9729f55b713/pandas/tests/indexes/period/test_constructors.py#L229
  14. comparison-with-callable:
    Comparing against a callable, did you omit the parenthesis?
    https://github.com/pandas-dev/pandas/blob/7ebc3e809491cdec3ffe4dc8917ad9729f55b713/pandas/tests/indexes/numeric/test_numeric.py#L79
  15. comparison-with-callable:
    Comparing against a callable, did you omit the parenthesis?
    https://github.com/pandas-dev/pandas/blob/7ebc3e809491cdec3ffe4dc8917ad9729f55b713/pandas/tests/indexes/numeric/test_numeric.py#L86
  16. comparison-with-callable:
    Comparing against a callable, did you omit the parenthesis?
    https://github.com/pandas-dev/pandas/blob/7ebc3e809491cdec3ffe4dc8917ad9729f55b713/pandas/tests/indexes/numeric/test_numeric.py#L90
  17. comparison-with-callable:
    Comparing against a callable, did you omit the parenthesis?
    https://github.com/pandas-dev/pandas/blob/7ebc3e809491cdec3ffe4dc8917ad9729f55b713/pandas/tests/indexes/numeric/test_numeric.py#L94
  18. comparison-with-callable:
    Comparing against a callable, did you omit the parenthesis?
    https://github.com/pandas-dev/pandas/blob/7ebc3e809491cdec3ffe4dc8917ad9729f55b713/pandas/tests/indexes/numeric/test_numeric.py#L98
  19. comparison-with-callable:
    Comparing against a callable, did you omit the parenthesis?
    https://github.com/pandas-dev/pandas/blob/7ebc3e809491cdec3ffe4dc8917ad9729f55b713/pandas/tests/indexes/numeric/test_numeric.py#L102
  20. redefined-variable-type:
    Redefinition of exp type from pandas.core.indexes.base.Index to pandas.core.indexes.numeric.NumericIndex
    https://github.com/pandas-dev/pandas/blob/7ebc3e809491cdec3ffe4dc8917ad9729f55b713/pandas/tests/indexes/numeric/test_numeric.py#L270
  21. comparison-with-callable:
    Comparing against a callable, did you omit the parenthesis?
    https://github.com/pandas-dev/pandas/blob/7ebc3e809491cdec3ffe4dc8917ad9729f55b713/pandas/tests/indexes/numeric/test_numeric.py#L304
  22. comparison-with-callable:
    Comparing against a callable, did you omit the parenthesis?
    https://github.com/pandas-dev/pandas/blob/7ebc3e809491cdec3ffe4dc8917ad9729f55b713/pandas/tests/indexes/numeric/test_numeric.py#L308
  23. redefined-variable-type:
    Redefinition of expected type from pandas.core.indexes.base.Index to pandas.core.indexes.numeric.NumericIndex
    https://github.com/pandas-dev/pandas/blob/7ebc3e809491cdec3ffe4dc8917ad9729f55b713/pandas/tests/indexes/numeric/test_numeric.py#L467
  24. comparison-with-callable:
    Comparing against a callable, did you omit the parenthesis?
    https://github.com/pandas-dev/pandas/blob/7ebc3e809491cdec3ffe4dc8917ad9729f55b713/pandas/tests/indexes/numeric/test_numeric.py#L486
  25. comparison-with-callable:
    Comparing against a callable, did you omit the parenthesis?
    https://github.com/pandas-dev/pandas/blob/7ebc3e809491cdec3ffe4dc8917ad9729f55b713/pandas/tests/indexes/numeric/test_numeric.py#L554
  26. comparison-with-callable:
    Comparing against a callable, did you omit the parenthesis?
    https://github.com/pandas-dev/pandas/blob/7ebc3e809491cdec3ffe4dc8917ad9729f55b713/pandas/tests/indexes/numeric/test_numeric.py#L558
  27. redefined-variable-type:
    Redefinition of idx type from pandas.core.indexes.numeric.UInt64Index to pandas.core.indexes.base.Index
    https://github.com/pandas-dev/pandas/blob/7ebc3e809491cdec3ffe4dc8917ad9729f55b713/pandas/tests/indexes/numeric/test_numeric.py#L619
  28. comparison-with-callable:
    Comparing against a callable, did you omit the parenthesis?
    https://github.com/pandas-dev/pandas/blob/7ebc3e809491cdec3ffe4dc8917ad9729f55b713/pandas/tests/indexes/numeric/test_numeric.py#L640
  29. comparison-with-callable:
    Comparing against a callable, did you omit the parenthesis?
    https://github.com/pandas-dev/pandas/blob/7ebc3e809491cdec3ffe4dc8917ad9729f55b713/pandas/tests/indexes/numeric/test_numeric.py#L644
  30. comparison-with-callable:
    Comparing against a callable, did you omit the parenthesis?
    https://github.com/pandas-dev/pandas/blob/7ebc3e809491cdec3ffe4dc8917ad9729f55b713/pandas/tests/indexing/test_coercion.py#L342
  31. no-member:
    Method 'dtype' has no '_resolution_obj' member
    https://github.com/pandas-dev/pandas/blob/7ebc3e809491cdec3ffe4dc8917ad9729f55b713/pandas/core/indexes/period.py#L172
  32. comparison-with-callable:
    Comparing against a callable, did you omit the parenthesis?
    https://github.com/pandas-dev/pandas/blob/7ebc3e809491cdec3ffe4dc8917ad9729f55b713/pandas/core/indexes/period.py#L407
  33. comparison-with-callable:
    Comparing against a callable, did you omit the parenthesis?
    https://github.com/pandas-dev/pandas/blob/7ebc3e809491cdec3ffe4dc8917ad9729f55b713/pandas/core/indexes/period.py#L455

Effect on psycopg:
The following messages are now emitted:

  1. no-member:
    Instance of 'ByteaLoader' has no '_escaping' member
    https://github.com/psycopg/psycopg/blob/d7a3785804ac97e139355e52bded734a13b62f26/psycopg/psycopg/types/string.py#L151

This comment was generated for commit f05d46e

@kriek kriek changed the title Add functional test demonstrating issue with type introspection Make the order of functional tests deterministic Oct 17, 2022
@Pierre-Sassoulas Pierre-Sassoulas merged commit d591cce into pylint-dev:main Oct 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Pierre-Sassoulas Pierre-Sassoulas Pierre-Sassoulas approved these changes

Assignees
No one assigned
Labels
Bug 🪲 Skip news 🔇 This change does not require a changelog entry
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kriek @Pierre-Sassoulas @DanielNoord @coveralls