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

start linting test files #3600

Merged
merged 15 commits into from Jan 9, 2023
Merged

start linting test files #3600

merged 15 commits into from Jan 9, 2023

Conversation

timmysilv
Copy link
Contributor

@timmysilv timmysilv commented Jan 5, 2023

Context:
pylint doesn't run for our test files for historical-only reasons, which isn't great. A full solution to this build-up of linting errors will take time, but a first step can be taken! We started talking about this today because we found two tests with the same name in some files, and that can cause unexpected behaviour. Idek if pylint catches that, but either way it sparked a conversation.

Description of the Change:

  • Adds an extended .pylintrc file for tests specifically, and a whitelist of test files that are currently passing pylint
  • Adds a step to the pylint GitHub action to run pylint on those whitelisted test files to ensure they remain linted
  • Adds a step to the pylint GitHub action to check if the PR adds any new test files, and if so, ensure that they are whitelisted. This will ensure that all new test files must meet our linting standard before being added to the codebase
  • Adds a make lint and make lint-test target to run pylint on the respective parts of the codebase
  • Add # pylint:disable comments (and one actual linting fix!) to files in pennylane/ to ensure the above-mentioned make lint passes locally at the time of this PR
    • I'm not sure why I was seeing errors on my machine (using this repo's pylintrc) if CI passes... whatever
    • make lint-test is ugly today - should the target only run on whitelisted files?

Benefits:
Our test files will be much cleaner, more readable, and sometimes more correct!

Possible Drawbacks:

  • This is only step 1. There is more work to be done to clean up our tests
  • Users may be confused with these new expectations (tests passing, how to whitelist new files)

Other Remarks

  • I'm fine to rename/move any of these files, just tried to give them intuitive-sounding and non-intrusive metadata
  • I can't figure out how to add the test whitelist to the pre-commit hook - it only accepts a filename regex which I can't get dynamically by reading the file in the pre-commit-config.yml file

@github-actions
Copy link
Contributor

github-actions bot commented Jan 5, 2023

Hello. You may have forgotten to update the changelog!
Please edit doc/releases/changelog-dev.md with:

  • A one-to-two sentence description of the change. You may include a small working example for new features.
  • A link back to this PR.
  • Your name (or GitHub username) in the contributors section.

@codecov
Copy link

codecov bot commented Jan 5, 2023

Codecov Report

Merging #3600 (9d2f4ff) into master (6c74b0e) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #3600   +/-   ##
=======================================
  Coverage   99.86%   99.86%           
=======================================
  Files         313      313           
  Lines       28075    28075           
=======================================
  Hits        28038    28038           
  Misses         37       37           
Impacted Files Coverage Δ
pennylane/collections/apply.py 100.00% <ø> (ø)
pennylane/collections/dot.py 100.00% <ø> (ø)
pennylane/data/dataset.py 100.00% <ø> (ø)
pennylane/optimize/adaptive.py 100.00% <ø> (ø)
pennylane/optimize/rotoselect.py 100.00% <ø> (ø)
pennylane/qaoa/cost.py 100.00% <ø> (ø)
pennylane/qaoa/cycle.py 100.00% <ø> (ø)
pennylane/qaoa/mixers.py 100.00% <ø> (ø)
pennylane/math/is_independent.py 100.00% <100.00%> (ø)
...nnylane/templates/layers/particle_conserving_u2.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@trbromley
Copy link
Contributor

For new tests, will they have to be added to tests_passing_pylint.txt?

@timmysilv
Copy link
Contributor Author

@trbromley still thinking about what to do with new tests but maybe

Copy link

@apotapov apotapov left a comment

Choose a reason for hiding this comment

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

Nice!

Would be good to give a way for devs to run this locally. Perhaps a shell script?

.github/tests/.pylintrc Outdated Show resolved Hide resolved
Copy link

@apotapov apotapov left a comment

Choose a reason for hiding this comment

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

lgtm, but would be good to have someone more knowledgeable to approve

@timmysilv
Copy link
Contributor Author

Here's an example run that fails when I added a new test file but didn't whitelist it, and the passing run once I whitelisted it.

.github/tests/.pylintrc Outdated Show resolved Hide resolved
@timmysilv timmysilv marked this pull request as ready for review January 5, 2023 22:18
@timmysilv timmysilv changed the title create a whitelist of files that pass pylint start linting test files Jan 5, 2023
@timmysilv timmysilv enabled auto-merge (squash) January 9, 2023 20:11
.github/workflows/scripts/validate_new_tests.sh Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@timmysilv timmysilv enabled auto-merge (squash) January 9, 2023 20:13
@timmysilv timmysilv merged commit 359130c into master Jan 9, 2023
@timmysilv timmysilv deleted the pylint-whitelist branch January 9, 2023 20:37
@apotapov apotapov mentioned this pull request Jan 10, 2023
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.

None yet

4 participants