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

Include Tests in static code analysis #558

Merged
merged 14 commits into from
May 6, 2022

Conversation

SteveDMurphy
Copy link
Contributor

@SteveDMurphy SteveDMurphy commented May 4, 2022

Closes #535

Code Changes

  • change --no-tty to -T so checks run again
  • disable irrelevant pylint checks across tests
  • resolve remaining pylint issues
  • include tests as part of pylint checks
  • include tests as part of xenon checks
  • include tests as part of our mypy checks
  • resolve any new isort checks
  • resolve and new black checks

Steps to Confirm

  • Updated and ran the static tests individually multiple times until getting the all clear

Pre-Merge Checklist

  • All CI Pipelines Succeeded
  • Documentation Updated:
    • documentation complete, or draft/outline provided (tag docs-team to complete/review on this branch)
    • documentation issue created (tag docs-team to complete issue separately)
  • Issue Requirements are Met
  • Relevant Follow-Up Issues Created
  • Update CHANGELOG.md

Description Of Changes

pylint

To make this work, we needed to disable checks for missing-docstring and redefined-outer-name. The first for being irrelevant and the second for conflicting with how pytest actually works, returning a false negative. The remainder of the changes are potential corrections for the relevant pylint errors remaining.

mypy

At the first pass, over 200 errors were found across more than 20 files. This was more of a blitz to get all of the type hinting correct without taking time to look for any other testing consolidation etc.

To make this work, we needed to disable checks for missing-docstring and redefined-outer-name. The first for being irrelevant and the second for conflicting with how pytest actually works, returning a false negative. The remainder of the changes are potential corrections for the relevant pylint errors remaining.
@SteveDMurphy SteveDMurphy self-assigned this May 5, 2022
@SteveDMurphy SteveDMurphy added this to the 1.7.0 milestone May 5, 2022
@SteveDMurphy SteveDMurphy added the dev experience Targets the developer experience label May 5, 2022
@SteveDMurphy
Copy link
Contributor Author

@ThomasLaPiana I found that the CI checks were passing with just a help message as part of this PR, currently working through it but may need a second set of eyes if I get stuck on anything here much longer. We may want to hold off on other dependency version bumping as well

@SteveDMurphy SteveDMurphy force-pushed the SteveDMurphy-535-testing-of-tests branch from 60bbdaa to 584b780 Compare May 5, 2022 14:38
SteveDMurphy and others added 2 commits May 5, 2022 13:27
@SteveDMurphy SteveDMurphy marked this pull request as ready for review May 5, 2022 18:06
@SteveDMurphy SteveDMurphy requested a review from a team May 5, 2022 18:06
@SteveDMurphy
Copy link
Contributor Author

Thanks for the help on the external test issue I caused @earmenda ! @ThomasLaPiana this one is large in file size but really the only test_ file changes should be for the static checks. The nox changes in particular had the most impact, I tried to separate out some commits to help a little with reviewing but sorry for the PR size! 😓

@ThomasLaPiana
Copy link
Contributor

@SteveDMurphy no worries! it do be like that sometimes :)

Thanks for the changes and the catch on that tricky nox bug!

noxfiles/ci_nox.py Outdated Show resolved Hide resolved
@ThomasLaPiana
Copy link
Contributor

pulled locally and testing now

@ThomasLaPiana
Copy link
Contributor

@SteveDMurphy herculean effort, thanks for knocking this one out!

@ThomasLaPiana
Copy link
Contributor

confirmed nox -s check_all -- docker works

Copy link
Contributor

@ThomasLaPiana ThomasLaPiana left a comment

Choose a reason for hiding this comment

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

LGTM!

@SteveDMurphy SteveDMurphy merged commit be94b37 into main May 6, 2022
@SteveDMurphy SteveDMurphy deleted the SteveDMurphy-535-testing-of-tests branch May 6, 2022 03:53
SteveDMurphy added a commit that referenced this pull request May 6, 2022
This was missed in #558 as part of the static code validation updates. Updating the param should fix the docs build failure, no update to the check should be necessary at this point.
ThomasLaPiana pushed a commit that referenced this pull request May 6, 2022
This was missed in #558 as part of the static code validation updates. Updating the param should fix the docs build failure, no update to the check should be necessary at this point.
ThomasLaPiana pushed a commit that referenced this pull request Aug 17, 2022
ThomasLaPiana pushed a commit that referenced this pull request Sep 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dev experience Targets the developer experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add static code analysis to tests/
3 participants