Skip to content

tests_functional rc file not found overpasses default disables hotfix #5687

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

Conversation

orSolocate
Copy link
Contributor

@orSolocate orSolocate commented Jan 15, 2022

Type of Changes

Type
βœ“ πŸ› Bug fix
✨ New feature
πŸ”¨ Refactoring
βœ“ πŸ“œ Docs

Description

When running tests_functional and there is no .rc file, the 3 default messages disables do not apply [suppressed-message, locally-disabled, useless-suppression].

Problematic code block of pylint/testutils/lint_module_test.py:

rc_file: Union[Path, str] = PYLINTRC
        try:
            rc_file = test_file.option_file
            self._linter.disable("suppressed-message")
            self._linter.disable("locally-disabled")
            self._linter.disable("useless-suppression")
        except NoFileError:
            pass

Suggested fix after discussion in this comment

Closes #XXX

@DanielNoord
Copy link
Collaborator

Copying my response from the other PR:

This is the relevant code:
https://github.com/PyCQA/pylint/blob/fdd71ee0cad6dc965c4a7bd7bc3d16e2ab564f70/pylint/testutils/lint_module_test.py#L47-L63
It sets rc_file to testing_pylintrc which includes the disables. Then it checks if there is a specific .rc file for the tests and overwrite rc_file. To make sure we still disable these three messages we then disable them manually on L50-52. I think this should disable all three in all cases, but I would obviously welcome a fix if I'm missing something!

The .rc file that can be missing is the .rc file for the individual test. The testing_pylintrc shouldn't be missing if the local branch has been made from a commit after efe59ca was merged into main.

@coveralls
Copy link

coveralls commented Jan 15, 2022

Pull Request Test Coverage Report for Build 1780787896

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

Totals Coverage Status
Change from base Build 1780124211: 0.0%
Covered Lines: 14759
Relevant Lines: 15722

πŸ’› - Coveralls

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 for the fix !

@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.13.0 milestone Jan 15, 2022
@Pierre-Sassoulas Pierre-Sassoulas added Bug πŸͺ² Needs review πŸ” Needs to be reviewed by one or multiple more persons labels Jan 15, 2022
@orSolocate
Copy link
Contributor Author

@DanielNoord I understand it better now.
I think the case when it happened to me is installing pylint dev repo as a package from the setup.py file and then running pytest from there. the testing_pylintrc is not created/exists for me...
This might be another bug but my solution at least solves it for these default disables..
can you please try and reproduce it? to check if the testing_pylintrc exists under the installed DEV pylint package?

@orSolocate
Copy link
Contributor Author

@Pierre-Sassoulas I think it can be pushed anyway.. because you want these default disables anyway if you have/don't have testing_pylintrc or if you have/don't have functional tests specific .rc file

@DanielNoord
Copy link
Collaborator

@DanielNoord I understand it better now.
I think the case when it happened to me is installing pylint dev repo as a package from the setup.py file and then running pytest from there. the testing_pylintrc is not created/exists for me...

I'm not sure how this works, sorry I'm not totally up to speed with the various ways pip and setup.py let's you install packages.
Which specific command are you using?
If it's:
pip install --upgrade pylint --pre this is indeed not working since the latest uploaded pre-release doesn't include the relevant commit.

@Pierre-Sassoulas I think it can be pushed anyway.. because you want these default disables anyway if you have/don't have testing_pylintrc or if you have/don't have functional tests specific .rc file

The latter case should be handled by the current code. The former is trickier: I don't know yet what type of dev package you're installing but since it apparently doesn't include the commit that added testing_pylintrc I don't think we should "support" it, or at least update that installation method.
Put differently, we shouldn't support a missing testing_pylintrc because it might indicate that other relevant commits might also be missing. This is not to say that I don't want to investigate why it is missing in your local environment, but I'd rather fix the "missing of the file" than adding a fix to allow it being missing.

@orSolocate
Copy link
Contributor Author

orSolocate commented Jan 21, 2022

I do pip uninstall pylint
then install the current repo as a package (this is what I mean by "dev"):
python setup.py install
In other words: I install Pylint that was cloned from this repo as a package.

I think that it has to do with the setup.py installation not creating the testing_pylintrc file...

The config file C:\Users\Or\Dev\python\pylint_py38\lib\site-packages\pylint-2.13.0.dev0-py3.8.egg\pylint\testutils\testing_pylintrc doesn
't exist!

Then when I use pytest i get the problem..

@orSolocate
Copy link
Contributor Author

close then and open a new issue for the testing_pylintrc?

@orSolocate
Copy link
Contributor Author

Hey guys I found the solution for it!

You need to install the package with:
python setup.py develop

instead of with:
python setup.py install

You get symlinks to the actual source code instead of creating a new directory. in the original directory obviously you have the testing_pylintrc file so it works smoothly.
moreover, you need this file only for testing, the end-user doesn't need this file. This is why I don't think that copying it to the new package directory benefits anyone..

I will delete my changes and add documentation for the correct installation of pylint while developing.

orSolocate added a commit to orSolocate/pylint that referenced this pull request Jan 29, 2022
@orSolocate orSolocate force-pushed the orb_pylintrc_tests_functional_hotfix branch from 7326eeb to e68ae41 Compare January 29, 2022 13:21
@DanielNoord
Copy link
Collaborator

Hmm, I wonder if we should add this to the docs. I always use git clone which seems to work fine. Is there a benefit of using python setup compared to git clone?

@orSolocate
Copy link
Contributor Author

orSolocate commented Jan 29, 2022

I also use git clone but then I install Pylint using python setup whenever I want to test my code before pushing it..
Can you please look at what I added to docs and see if it makes sense to you?

@DanielNoord
Copy link
Collaborator

I tend to use pip install -e. for this, which installs pylint as installable package in the current environment. If anything, I think we should promote that.

@Pierre-Sassoulas Do you have any opinion on this?

@Pierre-Sassoulas
Copy link
Member

I tend to use pip install -e.

Same here, it's the reason why we can't have pyproject.toml before PEP660 is out btw. I checked It's documented that way in the readme : https://github.com/PyCQA/pylint#testing.

orSolocate added a commit to orSolocate/pylint that referenced this pull request Jan 29, 2022
@orSolocate orSolocate force-pushed the orb_pylintrc_tests_functional_hotfix branch from e68ae41 to 19afddf Compare January 29, 2022 20:06
@orSolocate
Copy link
Contributor Author

pip install -e <repo_name> works the same. I added that in the docs to use either of these options.

@DanielNoord
Copy link
Collaborator

I think we should only recommend using the editable install. It seems to work fine without any issues both inside and outside of the fork and thus allows for some simplification, or is there something that python setup does that I'm missing?

@orSolocate
Copy link
Contributor Author

As far as I am concerned these 2 options are the same. they will give you the same results.. they both will override existing installation when invoked too.
The setup.py develop also work inside and outside the fork in the same way.. so it's really the same.
If you still think the 2nd option should be removed you can modify and commit the docs file

@orSolocate
Copy link
Contributor Author

@Pierre-Sassoulas should we merge it/modify message?

@Pierre-Sassoulas
Copy link
Member

I guess more doc can't hurt, but what's wrong with editable install ? Having only one way to do it would prevent us from having to troubleshoot the setup.py develop if something goes bad for a contributor with it. @DanielNoord what do you think ?

@orSolocate
Copy link
Contributor Author

I like that the repo tells the contributors how to set their environment, and having a straight way could be indeed better for troubleshooting. I am convinced now it's better this way. I will modify the message re-upload it

@orSolocate orSolocate force-pushed the orb_pylintrc_tests_functional_hotfix branch from 19afddf to 19b5766 Compare February 1, 2022 21:25
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 @orSolocate !

@Pierre-Sassoulas Pierre-Sassoulas merged commit 7e1eec2 into pylint-dev:main Feb 2, 2022
@orSolocate orSolocate deleted the orb_pylintrc_tests_functional_hotfix branch February 2, 2022 20:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug πŸͺ² Needs review πŸ” Needs to be reviewed by one or multiple more persons
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants