Skip to content

test_run_pylint_config: ignore pytest args #7165

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

Conversation

AdamWill
Copy link
Contributor

Type of Changes

Type
βœ“ πŸ› Bug fix

Description

This test fails if run through pytest with any args, because
_run_pylint_config passes through args from the command line
without checking if the args are actually intended for
pylint-config. We have to pass it some fake args that evaluate
truth-y but don't break anything, to avoid this.

Signed-off-by: Adam Williamson [email protected]

This test fails if run through pytest with any args, because
`_run_pylint_config` passes through args from the command line
without checking if the args are actually intended for
pylint-config. We have to pass it some fake args that evaluate
truth-y but don't break anything, to avoid this.

Signed-off-by: Adam Williamson <[email protected]>
@AdamWill
Copy link
Contributor Author

AdamWill commented Jul 11, 2022

Didn't add a changelog entry as the change is only to a test. To see the problem here, just try:

pytest -v tests/config/pylint_config/test_run_pylint_config.py

and watch it fail because pylint-config tries to handle the filename arg and gives unexpected output (_run_pylint_config passes through args starting from the second one, so we hit the bug if there's more than one arg). With this change, the test passes no matter what args you give pytest.

@AdamWill
Copy link
Contributor Author

I guess technically it might be better to change _run_pylint_config somehow, but as it's only used in two ways - in this test, and as the entry point for the pylint-config command - I don't think it's really necessary?

@coveralls
Copy link

Pull Request Test Coverage Report for Build 2652920634

  • 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 95.4%

Totals Coverage Status
Change from base Build 2650762732: 0.0%
Covered Lines: 16778
Relevant Lines: 17587

πŸ’› - Coveralls

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.

Brilliant, I saw this from time to time but never found the energy to look into it.

@jacobtylerwalls jacobtylerwalls added the Skip news πŸ”‡ This change does not require a changelog entry label Jul 12, 2022
@jacobtylerwalls jacobtylerwalls added this to the 2.15.0 milestone Jul 12, 2022
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.

This bothered me for months literally. Test worked as a standalone but not when launching everything and I thought there was a lot of work to fix it. Great job!

@Pierre-Sassoulas Pierre-Sassoulas merged commit 5b034c3 into pylint-dev:main Jul 12, 2022
@DanielNoord
Copy link
Collaborator

Worked on my machine πŸ˜…

Thanks for fixing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip news πŸ”‡ This change does not require a changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants