Start linting tests with black, isort and mypy and exclude old test files #1624

MVrachev · 2021-10-19T12:50:29Z

Addresses #1582

There will be another pr for mypy.

Description of the changes being introduced by the pull request:

Currently, we are using 4 tools: black, pylint, isort and mypy.
We want to run all 4 of them on our tests for the new codebase to make
it more readable and maintainable, but we don't want to run it on the
old test files as they are following the old style guidelines.

In order to achieve that we want to use an exclusion list instead of
an inclusion list as the new code base will be developed in the future
and new test files will appear. On another hand, we don't expect any
more additional test files against the old code, so this list is static.

I decided to hardcode the names we want to exclude because that way we
won't have to remove the Git history, as opposed to renaming or moving
the old test files.
Even though the list is big, 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 static content.

Signed-off-by: Martin Vrachev mvrachev@vmware.com

Please verify and check that the pull request fulfills the following
requirements:

The code follows the Code Style Guidelines
Tests have been added for the bug fix or new feature
Docs have been added for the bug fix or new feature

coveralls · 2021-10-19T12:54:23Z

Pull Request Test Coverage Report for Build 1381758103

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+1.1%) to 98.477%

Totals
Change from base Build 1380118579:	1.1%
Covered Lines:	3782
Relevant Lines:	3808

💛 - Coveralls

MVrachev · 2021-10-19T12:54:59Z

For context, I am planning to add this list of excluded files in setup.cfg for mypy.
Unfortunately, mypy doesn't have good options to exclude multiple files beautifully and that's why I had to use
a regex like this:

exclude = (tests\/aggregate_tests.py|tests\/fast_server_exit.py|tests\/simple_https_server.py|tests\/simple_server.py|tests\/slow_retrieval_server.py|tests\/utils.py|test_utils.py|test_repository_lib.py|test_arbitrary_package
_attack.py|test_developer_tool.py|test_download.py|test_endless_data_attack.py|test_extraneous_dependencies_attack.py|test_formats.py|test_indefinite_freeze_attack.py|test_key_revocation_integration.py|test_keydb.py|test_log.p
y|test_mirrors.py|test_mix_and_match_attack.py|test_multiple_repositories_integration.py|test_repository_tool.py|test_replay_attack.py|test_roledb.py|test_root_versioning_integration.py|test_sig.py|test_slow_retrieval_attack.p
y|test_tutorial.py|test_unittest_toolbox.py|test_updater.py|test_updater_root_rotation_integration.py|tests\/repository_data\/generate_project_data.py|tests\/repository_data\/generate.py)

which is ugly, but it's not complicated to understand and it works.

jku

I think doing all the linters at once is the correct choice (so we get to see the whole configuration at once) but instead of including changes in 8 test files maybe one representative file would be a good start -- the patch becomes quite big?

Mostly it looks good though. There seem to be quite a few pylint disables in the code: I can understand some of them (e.g. test data is longer than line length) but not really others. I saw at least these that seemed like they should be fixed:

invalid-name: let's fix the variable names or lets change the pylint config to accept "_" as dummy variable: adding individual disables in the code is just spam).
consider-iterating-dictionary
redefined-builtin
no-self-use

With regards to how we choose what to lint: four different almost identical lists in three different files is not great... maybe it's not the end of he world but let's keep an open mind on the possibility of moving files as well.

jku · 2021-10-20T12:19:29Z

tests/repository_simulator.py

@@ -174,6 +184,7 @@ def publish_root(self):
        logger.debug("Published root v%d", self.root.version)

    def fetch(self, url: str) -> Iterator[bytes]:
+        """Fetches data from the given url and yeilds an Iterator"""


returns an Iterator (or yields bytes).

jku · 2021-10-21T07:50:42Z

@lukpueh I forget, have we discussed dummy-variables-rgx in pylintrc at some point? I rather like using _ (in places like prefix, _, name = filename.rpartition(".")) and somehow I'm always surprised when it does not work in python-tuf (you'd think I'd remember by now but no...).

It's not a big deal though: if this is a conscious decision then let's just rename the variables -- there's just one or two anyway

lukpueh · 2021-10-21T08:22:26Z

@jku, I don't think we have. Does pylint choke on _? Briefly looking at pylintrc and tuf/api/pylintrc it shouldn't. If it does, let's revise the patterns. IMHO _ is rather idiomatic (it's also a pylint default)

FYI, it seems like in the past junk was preferred over _ (see old style guide and e.g. ceb50dd). But our revised style guide does not have this preference anymore.

MVrachev · 2021-10-21T08:26:08Z

I think doing all the linters at once is the correct choice (so we get to see the whole configuration at once) but instead of including changes in 8 test files maybe one representative file would be a good start -- the patch becomes quite big?

I can do that, but also some of the cases and disabled errors of the tests won't be seen that way.

Mostly it looks good though. There seem to be quite a few pylint disables in the code: I can understand some of them (e.g. test data is longer than line length) but not really others.

I tried to make sensible decisions for that, but each of the disabled errors should be examined individually.

With regards to how we choose what to lint: four different almost identical lists in three different files is not great... maybe it's not the end of he world but let's keep an open mind on the possibility of moving files as well.

Yea, I know it's not beautiful. The problem is each of the test files has it's own format for excluding multiple files.
I wanted to experiment with using pyproject.yml for pylint and mypy as well, but they already have their own configuration files.
I was thinking of a way to define the exclusion list/lists in one file and somehow import it into the configuration files.
Haven't really researched for that. Do you think that would be better @jku?
Another option is to research if we can move pylint or mypy configuration options inside pyproject.yml and use this one alone?

I will do some research on those items later today.

jku · 2021-10-21T08:39:38Z

I was thinking of a way to define the exclusion list/lists in one file and somehow import it into the configuration files.

Looking at how different the formats are, this will probably get difficult and ugly very quickly.

Another option is to research if we can move pylint or mypy configuration options inside pyproject.yml and use this one alone?

that would be better than multiple files (still not great but a lot better)

jku · 2021-10-21T08:53:30Z

Does pylint choke on _? Briefly looking at pylintrc and tuf/api/pylintrc it shouldn't.

Hmm, you have a point, I'm not sure what the issue is exactly.

I do remember stumbling on this before and based on the patch Martin did now as well: @MVrachev if you can figure out which rule forced you to use disable=invalid-name that would be good. It may be that we need to include "_" in good-names: comparing with upstream pylintrc might be helpful

MVrachev · 2021-10-25T15:34:04Z

@jku I have good news: I was able to move both pylint and mypy configurations to pyproject.toml.
For mypy: I was successful in adding an exclusion list in an almost identical manner as I was going to add it in setup.cfg:

exclude = "(tests/aggregate_tests.py|tests/fast_server_exit.py|tests/simple_https_server.py|tests/simple_server.py|tests/slow_retrieval_server.py|tests/utils.py|test_utils.py|test_repository_lib.py|test_arbitrary_package_attack.py|test_developer_tool.py|test_download.py|test_endless_data_attack.py|test_extraneous_dependencies_attack.py|test_formats.py|test_indefinite_freeze_attack.py|test_key_revocation_integration.py|test_keydb.py|test_log.py|test_mirrors.py|test_mix_and_match_attack.py|test_multiple_repositories_integration.py|test_repository_tool.py|test_replay_attack.py|test_roledb.py|test_root_versioning_integration.py|test_sig.py|test_slow_retrieval_attack.py|test_tutorial.py|test_unittest_toolbox.py|test_updater.py|test_updater_root_rotation_integration.py|tests/repository_data/generate_project_data.py|tests/repository_data/generate.py)"

I didn't add it here as we had discussed that mypy changes will be added in a later pr.

After reading different issues It seems as after PEP 518 many projects opt-in using pyproject.toml as a place for multi-tool configuration.

So, @jku what do you think?

PS: I had to force-push so many times as I was not able to reproduce a mypy linting error from the CI.

Currently, we are using 4 tools: black, pylint, isort and mypy. We want to run all 4 of them on our tests for the new codebase to make it more readable and maintainable, but we don't want to run it on the old test files as they are following the old style guidelines. In order to achieve that we want to use an exclusion list instead of an inclusion list as the new code base will be developed in the future and new test files will appear. On another hand, we don't expect any more additional test files against the old code, so this list is static. I decided to hardcode the names we want to exclude because that way we won't have to remove the Git history, as opposed to renaming or moving the old test files. Even though the list is big, 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 static content. Signed-off-by: Martin Vrachev <mvrachev@vmware.com>

All of the changes included are a result of applying black on our tests on the new code. Signed-off-by: Martin Vrachev <mvrachev@vmware.com>

Signed-off-by: Martin Vrachev <mvrachev@vmware.com>

Currently, we are using 4 tools: black, pylint, isort and mypy. We want to run all 4 of them on our tests for the new codebase to make it more readable and maintainable, but we don't want to run it on the old test files as they are following the old style guidelines. I am using the same approach as the one used for black - create an exclusion list from old test files we want to ignore. I didn't used moved or renamed the old test files as that will remove the Git history. Signed-off-by: Martin Vrachev <mvrachev@vmware.com>

All of the changes included are a result of applying isort on our tests on the new code. Signed-off-by: Martin Vrachev <mvrachev@vmware.com>

Currently, we are using 4 tools: black, pylint, isort and mypy. We want to run all 4 of them on our tests for the new codebase to make it more readable and maintainable, but we don't want to run it on the old test files as they are following the old style guidelines. I am using the same approach as the one used for black - create an exclusion list from old test files we want to ignore. I didn't used moved or renamed the old test files as that will remove the Git history. Signed-off-by: Martin Vrachev <mvrachev@vmware.com>

Pylint reported a couple of warnings flagged as "duplicate-code". We were truly duplicating code - one of the examples was when we imported the same objects from tuf/api/metadata.py: MetaFile, Role, Root, Snapshot, TargetFile, Targets, and Timestamp in two separate modules. So, I thought we do want to be repetitive here and include that code at both modules. The problem is that besides importing the above classes the modules imported other classes from tuf.api.metadata.py and there was no way to disable this check. I searched and found out that this is a known problem: pylint-dev/pylint#214. That's why the only solution I see is to disable this warning temporarily and hoping that one day when this issue is fixed we will remember to turn it on again. Signed-off-by: Martin Vrachev <mvrachev@vmware.com>

Disable pylint line-too-long warning as test_metadata_serialization.py is a special case where having longer lines helps the readibility of the test cases data. Signed-off-by: Martin Vrachev <mvrachev@vmware.com>

Address or disable pylint warnings raised on all test files inside tests/ testing the code of the new implementation. Signed-off-by: Martin Vrachev <mvrachev@vmware.com>

_ is often used when a function returns multiple values and you need a sub-portion of them. Then, those values that are unnecessary can be named _. Currently, pylint warns us that this is not a good variable name, so fix that. Signed-off-by: Martin Vrachev <mvrachev@vmware.com>

Move configuration for pylint linting tool over the refactored code in pyproject.toml. This is done in order to have one unified place where we will have the 30+ lines exclusion list of old test files we don't want to lint. Signed-off-by: Martin Vrachev <mvrachev@vmware.com>

Move configuration for mypy linting tool over the refactored code in pyproject.toml. This is done in order to have one unified place where we will have the 30+ lines exclusion list of old test files we don't want to lint. Signed-off-by: Martin Vrachev <mvrachev@vmware.com>

MVrachev · 2021-10-27T21:21:51Z

It was not easy, but I think I resolved all of the conflicts.
You can have a look into the specific changes now @jku.

MVrachev · 2021-10-27T21:23:10Z

I noticed the CI run 19 tests here, but for #1636 it runs 20.
Any idea why that happens?

jku · 2021-10-29T08:56:03Z

You can have a look into the specific changes now @jku.

If you mean e.g. that I could go through all of the pylint: disable lines... I would prefer you do it: please try to fix issues instead of disabling lints if you can. If something is not a clear cut case, leave a review comment. After this I can review what's left -- so we make sure we agree that they are appropriate disables.

Actual issues in the code that you find could be fixed with separate PRs (that would get handled before this PR) or as part of this one -- the former approach will make them faster to merge. In fact even the code fixes already in this PR could easily be made in separate smaller PRs: we'd fix the issues first, and then enabling the new config would be easier to review as it would only include the config changes plus the required pylint: disable lines?

MVrachev · 2021-10-29T15:13:43Z

You can have a look into the specific changes now @jku.

If you mean e.g. that I could go through all of the pylint: disable lines... I would prefer you do it: please try to fix issues instead of disabling lints if you can. If something is not a clear cut case, leave a review comment. After this I can review what's left -- so we make sure we agree that they are appropriate disables.

I think I have used disables only for the places I find appropriate.
In most of the cases, I have followed the linters recommendations and applied them.
Still, I will have another look into them.

Actual issues in the code that you find could be fixed with separate PRs (that would get handled before this PR) or as part of this one -- the former approach will make them faster to merge. In fact, even the code fixes already in this PR could easily be made in separate smaller PRs: we'd fix the issues first, and then enabling the new config would be easier to review as it would only include the config changes plus the required pylint: disable lines?

I will do that on Monday and create new prs about the code changes that make sense from the linters and leave the disables and config changes in separate pr.
Still, it would have been better if I knew that's what you preferred all along and not create those two prs.

MVrachev · 2021-11-02T17:33:44Z

This pr will be replaced with a couple of other prs.
As Jussi has suggested, I will first introduce the automatic linting changes, then pylint and mypy changes and disables, and in the end, I will add a pr with the configuration options.

Closing this pr.

MVrachev requested review from jku and sechkova and removed request for jku October 19, 2021 12:50

MVrachev mentioned this pull request Oct 19, 2021

Apply black on tests #1623

Closed

MVrachev force-pushed the start-linting branch from ca91fef to b86a31c Compare October 19, 2021 14:18

jku reviewed Oct 20, 2021

View reviewed changes

MVrachev force-pushed the start-linting branch 4 times, most recently from b6bddd4 to 04b32f6 Compare October 25, 2021 15:29

MVrachev added 11 commits October 28, 2021 00:12

Apply black on the tests of the new code

1994bdf

All of the changes included are a result of applying black on our tests on the new code. Signed-off-by: Martin Vrachev <mvrachev@vmware.com>

Rename & simplify a couple of tests in test_api.py

fc77265

Signed-off-by: Martin Vrachev <mvrachev@vmware.com>

Apply isort on the tests of the new code

0281a87

All of the changes included are a result of applying isort on our tests on the new code. Signed-off-by: Martin Vrachev <mvrachev@vmware.com>

Pylint: disable line-too-long for a test file

dc65834

Disable pylint line-too-long warning as test_metadata_serialization.py is a special case where having longer lines helps the readibility of the test cases data. Signed-off-by: Martin Vrachev <mvrachev@vmware.com>

Address pylint warnings on tests for the new code

2fff0d8

Address or disable pylint warnings raised on all test files inside tests/ testing the code of the new implementation. Signed-off-by: Martin Vrachev <mvrachev@vmware.com>

MVrachev force-pushed the start-linting branch from 04b32f6 to 17fdcbe Compare October 27, 2021 21:19

MVrachev closed this Nov 2, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Start linting tests with black, isort and mypy and exclude old test files #1624

Start linting tests with black, isort and mypy and exclude old test files #1624

MVrachev commented Oct 19, 2021

coveralls commented Oct 19, 2021 •

edited

Loading

MVrachev commented Oct 19, 2021 •

edited

Loading

jku left a comment

jku Oct 20, 2021

jku commented Oct 21, 2021

lukpueh commented Oct 21, 2021

MVrachev commented Oct 21, 2021

jku commented Oct 21, 2021

jku commented Oct 21, 2021 •

edited

Loading

MVrachev commented Oct 25, 2021 •

edited

Loading

MVrachev commented Oct 27, 2021

MVrachev commented Oct 27, 2021

jku commented Oct 29, 2021 •

edited

Loading

MVrachev commented Oct 29, 2021 •

edited

Loading

MVrachev commented Nov 2, 2021

Start linting tests with black, isort and mypy and exclude old test files #1624

Start linting tests with black, isort and mypy and exclude old test files #1624

Conversation

MVrachev commented Oct 19, 2021

coveralls commented Oct 19, 2021 • edited Loading

Pull Request Test Coverage Report for Build 1381758103

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

MVrachev commented Oct 19, 2021 • edited Loading

jku left a comment

Choose a reason for hiding this comment

jku Oct 20, 2021

Choose a reason for hiding this comment

jku commented Oct 21, 2021

lukpueh commented Oct 21, 2021

MVrachev commented Oct 21, 2021

jku commented Oct 21, 2021

jku commented Oct 21, 2021 • edited Loading

MVrachev commented Oct 25, 2021 • edited Loading

MVrachev commented Oct 27, 2021

MVrachev commented Oct 27, 2021

jku commented Oct 29, 2021 • edited Loading

MVrachev commented Oct 29, 2021 • edited Loading

MVrachev commented Nov 2, 2021

coveralls commented Oct 19, 2021 •

edited

Loading

MVrachev commented Oct 19, 2021 •

edited

Loading

jku commented Oct 21, 2021 •

edited

Loading

MVrachev commented Oct 25, 2021 •

edited

Loading

jku commented Oct 29, 2021 •

edited

Loading

MVrachev commented Oct 29, 2021 •

edited

Loading