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

tests: Start linting tests #1582

Closed
jku opened this issue Sep 15, 2021 · 10 comments · Fixed by #1708 or #1710
Closed

tests: Start linting tests #1582

jku opened this issue Sep 15, 2021 · 10 comments · Fixed by #1708 or #1710
Assignees
Labels
backlog Issues to address with priority for current development goals testing
Milestone

Comments

@jku
Copy link
Member

jku commented Sep 15, 2021

I believe we should enable linting (tox -e lint) for new test files (maybe even for some recent additions). Possibly some exceptions are needed but in general I think the tests would be better if they were linted.

Good start for this would be test_updater_with_simulator.py and repository_simulator.py: it's new code and files are small so the changes are easy -- the real job is figuring out how to select what to lint... Possible options:

  • separate directory for legacy (unlinted) tests?
  • define list of linted files in tox.ini / setup.cfg ? (I don't like this one as it means default for new files is no lint)
  • define list of files excluded from lint in tox.ini & setup.cfg ?
  • some other strategy?
@joshuagl
Copy link
Member

Splitting tests across multiple directories may make sense, but I think the right combination of simple and not prone to error is a list of files excluded from lint? That way, new test files are linted and there's no ambiguity about where to place new tests.

Also, the no-lint list gets shorter as we replace/remove existing tests.

@jku
Copy link
Member Author

jku commented Sep 15, 2021

I notice I did not explicitly state this: we currently have lint files/directories listed in multiple places in tox.ini and then in setup.cfg for static type checking with mypy

@sechkova sechkova added backlog Issues to address with priority for current development goals testing labels Sep 29, 2021
@sechkova sechkova added this to the Sprint 9 milestone Sep 29, 2021
@MVrachev
Copy link
Collaborator

MVrachev commented Oct 8, 2021

I think I will experiment by renaming all old test files with <name>_old.py and experiment with exclusion rules.

@sechkova
Copy link
Contributor

sechkova commented Oct 8, 2021

I think I will experiment by renaming all old test files with <name>_old.py and experiment with exclusion rules.

Can we use a prefix instead of a suffix so the files appear sorted in a way?

@MVrachev
Copy link
Collaborator

MVrachev commented Oct 8, 2021

I think I will experiment by renaming all old test files with <name>_old.py and experiment with exclusion rules.

Can we use a prefix instead of a suffix so the files appear sorted in a way?

I thought about adding a suffix instead of a prefix as the filenames will be easier to read.
Although as they are test files for the old legacy code maybe we want to see them grouped and that will make the other files more visible.
Do the others have an opinion?

@jku
Copy link
Member Author

jku commented Oct 11, 2021

I expect prefixes would make more sense, if files need to be mass-renamed

@MVrachev
Copy link
Collaborator

MVrachev commented Oct 11, 2021

Another reasonable solution seems to move the old files in a separate folder, but if we move them won't we lose the git history?

@MVrachev
Copy link
Collaborator

MVrachev commented Oct 11, 2021

I read the GIT FAQ on this matter https://git.wiki.kernel.org/index.php/GitFaq#Why_does_Git_not_.22track.22_renames.3F and found out that both renames and file relocations will cause the git history to be lost.
That's why I researched for another option that won't change the titles or move the test files.
I think I have some progress with isort and black, but they require to hardcode each of the test files we are going to exclude. This makes sense when we consider they don't have a distinguished regex we can exclude.
Even though it's a big list of around 30 files, I think this solution is fine as this list will only contain files testing the old code meaning the list will have a static content.

@MVrachev
Copy link
Collaborator

MVrachev commented Nov 2, 2021

I will remove this issue from Sprint 11 for now as it contains multiple action items each of which requires a review.

@MVrachev
Copy link
Collaborator

MVrachev commented Nov 9, 2021

TODO items left to close this issue:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backlog Issues to address with priority for current development goals testing
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants