New implementation tests: address pylint warnings #1664

MVrachev · 2021-11-05T15:58:09Z

Related to #1657:

Description of the changes being introduced by the pull request:

I believe we should enable linting (tox -e lint) for new test files testing the new implementation.

The first step to do that would be to apply the automatic fixes proposed
by black and isort on the new test files testing the new code.
This happened in #1658.

The next step would be to propose fixes for pylint and mypy and the last
the part will be to enable the linters themselves.

In this pr, I include fixes for the warnings by pylint only as those changes are NOT
automatically by the tool and are more than enough for a pr.

coveralls · 2021-11-05T16:04:52Z

Pull Request Test Coverage Report for Build 1426425036

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.548%

Totals
Change from base Build 1426058326:	1.1%
Covered Lines:	3116
Relevant Lines:	3129

💛 - Coveralls

kairoaraujo · 2021-11-08T09:16:34Z

tests/repository_simulator.py

@@ -136,6 +139,7 @@ def targets(self) -> Targets:
        return self.md_targets.signed

    def all_targets(self) -> Iterator[Tuple[str, Targets]]:
+        """Yield role name and signed portion of targets one by one"""


I am not sure if relevant comment.
To be consistent with another docstring you had added, finish with a period.
If it makes sense, it would be interesting to apply the other changes that are part of this PR.

I will add . at the end of the docstrings.
Do you have a better suggestion for this docstring?
The function is simple and I am not sure what else can I write here.

I don't have a better suggestion for this docstring. LGTM

[General comment, not sure if the right place]
I think what we are doing in the ngclient for docstring is the best option. In a sense, it can become good documentation in the end (as part of readthedocs/sphinx).
I understand that this is a test part, but it can help a lot of new contributors. Any thoughts @jku ?

tests/repository_simulator.py

tests/test_api.py

kairoaraujo · 2021-11-08T09:45:47Z

tests/test_api.py

-        root_path = os.path.join(self.repo_dir, "metadata", "root.json")
-        root = Metadata[Root].from_file(root_path).signed
+        path = os.path.join(self.repo_dir, "metadata", "root.json")
+        root = Metadata[Root].from_file(path).signed


Above you changed from dir to dest_dir, I see it as more explicit.
At these lines, I see the root_path as path, any specific reason for this?

dir was likely changed because it's a builtin, this change is likely to keep lines below 80 chars

root_path was changed to path, because there was a pylint warning we were using too many variables in a single function and with this change, we can reuse path.
About dir Jussi is correct.

re-using variables is usually a code smell but this looks OK to me. The best solution might be a refactor of the function (to maybe multiple tests) but I appreciate not doing large changes under "address pylint warnings": anyway LGTM

This is no longer of a concern after the merge of #1671 and I removed this change.

tests/test_updater_ng.py

MVrachev · 2021-11-08T16:32:26Z

I addressed comments made by @kairoaraujo:

added . at the end of the docstrings
used one consistent format when mentioning a specific python file
small docstring improvements

jku

Thanks Martin, Kairo. LGTM.

jku · 2021-11-10T08:03:16Z

tests/test_api.py

-        root_path = os.path.join(self.repo_dir, "metadata", "root.json")
-        root = Metadata[Root].from_file(root_path).signed
+        path = os.path.join(self.repo_dir, "metadata", "root.json")
+        root = Metadata[Root].from_file(path).signed


re-using variables is usually a code smell but this looks OK to me. The best solution might be a refactor of the function (to maybe multiple tests) but I appreciate not doing large changes under "address pylint warnings": anyway LGTM

jku · 2021-11-10T08:12:04Z

I was a bout to merge ... but first merged another change that created a conflict. Sorry about that, could you resolve please?

MVrachev · 2021-11-10T12:36:41Z

Rebased on top of develop as there were conflicts after the merge of #1636.

MVrachev · 2021-11-10T14:01:30Z

I proposed a pr fixing pylint warning about too many local variables in test_sign_verify() by splitting it: #1671.
Maybe we can merge #1671 and then I will update this one without reusing path?

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>

_ 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>

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

MVrachev · 2021-11-11T13:41:40Z

#1671 is merged and I have rebased and removed the change which handled too many local variables (renaming root_path to path).

kairoaraujo reviewed Nov 8, 2021

View reviewed changes

MVrachev force-pushed the apply-pylint branch from 87c6c31 to ce10b69 Compare November 8, 2021 16:30

jku approved these changes Nov 10, 2021

View reviewed changes

MVrachev force-pushed the apply-pylint branch from ce10b69 to 2f96935 Compare November 10, 2021 12:34

MVrachev added 3 commits November 11, 2021 15:37

Address pylint warnings on tests for the new code

d4187f3

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

MVrachev force-pushed the apply-pylint branch from 2f96935 to d4187f3 Compare November 11, 2021 13:40

jku merged commit 41b20df into theupdateframework:develop Nov 11, 2021

MVrachev deleted the apply-pylint branch November 11, 2021 14:56

lukpueh mentioned this pull request Dec 13, 2021

Prepare v0.20.0 #1723

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New implementation tests: address pylint warnings #1664

New implementation tests: address pylint warnings #1664

MVrachev commented Nov 5, 2021

coveralls commented Nov 5, 2021 •

edited

Loading

kairoaraujo Nov 8, 2021

MVrachev Nov 8, 2021

kairoaraujo Nov 9, 2021 •

edited

Loading

kairoaraujo Nov 8, 2021

jku Nov 8, 2021

MVrachev Nov 8, 2021 •

edited

Loading

jku Nov 10, 2021

MVrachev Nov 11, 2021

MVrachev commented Nov 8, 2021

jku left a comment

jku Nov 10, 2021

jku commented Nov 10, 2021

MVrachev commented Nov 10, 2021

MVrachev commented Nov 10, 2021 •

edited

Loading

MVrachev commented Nov 11, 2021

New implementation tests: address pylint warnings #1664

New implementation tests: address pylint warnings #1664

Conversation

MVrachev commented Nov 5, 2021

coveralls commented Nov 5, 2021 • edited Loading

Pull Request Test Coverage Report for Build 1426425036

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

kairoaraujo Nov 8, 2021

Choose a reason for hiding this comment

MVrachev Nov 8, 2021

Choose a reason for hiding this comment

kairoaraujo Nov 9, 2021 • edited Loading

Choose a reason for hiding this comment

kairoaraujo Nov 8, 2021

Choose a reason for hiding this comment

jku Nov 8, 2021

Choose a reason for hiding this comment

MVrachev Nov 8, 2021 • edited Loading

Choose a reason for hiding this comment

jku Nov 10, 2021

Choose a reason for hiding this comment

MVrachev Nov 11, 2021

Choose a reason for hiding this comment

MVrachev commented Nov 8, 2021

jku left a comment

Choose a reason for hiding this comment

jku Nov 10, 2021

Choose a reason for hiding this comment

jku commented Nov 10, 2021

MVrachev commented Nov 10, 2021

MVrachev commented Nov 10, 2021 • edited Loading

MVrachev commented Nov 11, 2021

coveralls commented Nov 5, 2021 •

edited

Loading

kairoaraujo Nov 9, 2021 •

edited

Loading

MVrachev Nov 8, 2021 •

edited

Loading

MVrachev commented Nov 10, 2021 •

edited

Loading