Skip to content

More tests for imported modules #1399

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

Merged
merged 11 commits into from
May 11, 2022

Conversation

tristanlatr
Copy link
Contributor

@tristanlatr tristanlatr commented Feb 16, 2022

Type of Changes

This PR adds more tests, see #1398

Related Issue

Resolves #1398

@tristanlatr tristanlatr marked this pull request as draft February 16, 2022 20:23
@tristanlatr
Copy link
Contributor Author

tristanlatr commented Feb 17, 2022

The way the tests are faillinbg is very weird and I'm unsure if it's linked to my changes.

The following test fails:
tests/unittest_scoped_nodes.py::ClassNodeTest::test_mro_with_factories with a difference in the computed MRO:

E   - ['FinalClass', 'ClassB', 'MixinB', 'ClassA', 'MixinA', 'Base', 'object']
E   + ['FinalClass', 'ClassB', 'MixinB', '', 'ClassA', 'MixinA', '', 'Base', 'object']

I wonder if this is a side effect of calling clear_cache() or it's totally unrelated? Any clue?

@Pierre-Sassoulas
Copy link
Member

Ugh, I think you're right the tests might not be independent and rely on the caching between tests. Do you have an idea how to fix the failing test ?

@tristanlatr
Copy link
Contributor Author

I could restore the cache after the tests ?

@tristanlatr
Copy link
Contributor Author

Looks like it doesn’t work :/ I must be missing something…

@jacobtylerwalls
Copy link
Member

I was just working with astroid's caching mechanism recently and remembered using AstroidCacheSetupMixin to solve test isolation issues, so I just took the liberty to use it here :-)

@tristanlatr @Pierre-Sassoulas do you have thoughts about whether we should merge these tests?

@coveralls
Copy link

coveralls commented May 11, 2022

Pull Request Test Coverage Report for Build 2308678113

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 91.665%

Totals Coverage Status
Change from base Build 2303054388: 0.0%
Covered Lines: 9139
Relevant Lines: 9970

💛 - Coveralls

@tristanlatr tristanlatr marked this pull request as ready for review May 11, 2022 17:36
@tristanlatr
Copy link
Contributor Author

Hello @jacobtylerwalls,

Thanks a lot for your input here. If I understand correctly, AstroidCacheSetupMixin clear the cache between each test method, right ?

If all tests passes, I would be happy that these test land in the main branch.

@jacobtylerwalls
Copy link
Member

Not between each test method but at the beginning and end of the class. (The reason your test was failing was the same thing I saw and fixed in #1528.) Do we need to clear the cache with every test method? If so we can adjust the behavior of the mixin to use setUp and tearDown instead (probably in another PR).

The failure on 3.11 is unrelated and tracked in #1551

@jacobtylerwalls jacobtylerwalls changed the title More tests, see #1398 More tests for imported modules May 11, 2022
@jacobtylerwalls jacobtylerwalls added this to the 2.12.0 milestone May 11, 2022
Copy link
Member

@jacobtylerwalls jacobtylerwalls left a comment

Choose a reason for hiding this comment

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

I think we can merge this; if we want to increase the test isolation to clear caches every test method, we can do that in another PR anyway.

@jacobtylerwalls jacobtylerwalls merged commit 847f7be into pylint-dev:main May 11, 2022
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.

Cannot infer assignment with operator for a simple list of strings
4 participants