Start linting the test #1710

MVrachev · 2021-12-06T14:19:31Z

Description of the changes being introduced by the pull request:

Start linting the files that test the new code. In order to do that the old test files
had to be excluded. They were are renamed to test_*_old.py, so we can add globs/regex to exclude
the files testing the old code.
These options will be removed after we remove those files.

Additional black, pylint and mypy errors were found and had to be addressed
before we can start linting.

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-12-06T14:50:25Z

Pull Request Test Coverage Report for Build 1559295338

2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.0004%) to 97.582%

Totals
Change from base Build 1554266096:	0.0004%
Covered Lines:	4033
Relevant Lines:	4117

💛 - Coveralls

lukpueh

Thanks for the PR, @MVrachev! Looks like a straight-forward set of changes. I left some minor suggestions, but will approve regardless.

I see you adopted the filename changes (add "_old" infix) in some doc headers but not all. I suggest to do this consistently and I'm fine with not updating the doc headers, given that these files are bound to be removed.

lukpueh · 2021-12-07T09:28:31Z

tests/test_indefinite_freeze_attack_old.py

@@ -36,7 +36,6 @@
  metadata without the client being aware.
 """

-import datetime


Why this change in an "_old.py" file?

I just saw it was unused and decided to remove it, but I can return it if it looks strange.

🤷 Probably not worth the effort. :) Let's leave it there.

lukpueh · 2021-12-07T09:31:40Z

tests/test_updater_consistent_snapshot.py

@@ -117,6 +121,7 @@ def test_top_level_roles_update(self, test_case_data: Dict[str, Any]):
        sim = self._init_repo(consistent_snapshot)
        updater = self._init_updater(sim)

+        # pylint: disable=protected-access


Maybe we want to disable this globally for all tests?

I don't think that is possible as of today.
See issue pylint-dev/pylint#618 and their project where they keep track of the progress on that issue https://github.com/PyCQA/pylint/projects/1

You could call pylint separately for tests and for everything else in tox.ini, as we do for in-toto.

The only downside is that we will no longer have one source for the folders we lint or in other words we won't be able to use lint_dirs:

python-tuf/tox.ini

Line 41 in aebd8b0

lint_dirs = tuf/api tuf/ngclient examples

for pylint.
@jku any thoughts here as you suggested lint_dirs?

globally disabling the lint is ok ... but at this point might be overkill: As an example, there's already a PR that removes these protected accesses (because there is a better design for these cases): I think we won't have more than a couple in the whole test suite next week.

(this is a response to earlier comment)

lukpueh · 2021-12-07T10:04:08Z

Just merged #1708. Please rebase and mark ready for review!

MVrachev · 2021-12-07T10:36:40Z

Thanks for the PR, @MVrachev! Looks like a straight-forward set of changes. I left some minor suggestions, but will approve regardless.

I see you adopted the filename changes (add "_old" infix) in some doc headers but not all. I suggest to do this consistently and I'm fine with not updating the doc headers, given that these files are bound to be removed.

I rebased on top of develop and I think changed all the relevant doc headers and places where the file names of
the old test files were mentioned.
The pr is ready for review!

MVrachev · 2021-12-07T10:42:58Z

I had to rebase to change the test name used as an example in docs/CONTRIBUTIONS.rst.

jku

Moving almost obsolete files is a bit annoying but I can see why you want to flick the switch on this one :) style guidelines won't be observed without hard failures... LGTM in general

We should keep statically checking dateutil, otherwise my comments are just discussion.

pyproject.toml

jku · 2021-12-08T07:57:51Z

tests/test_updater_consistent_snapshot.py

@@ -117,6 +121,7 @@ def test_top_level_roles_update(self, test_case_data: Dict[str, Any]):
        sim = self._init_repo(consistent_snapshot)
        updater = self._init_updater(sim)

+        # pylint: disable=protected-access


globally disabling the lint is ok ... but at this point might be overkill: As an example, there's already a PR that removes these protected accesses (because there is a better design for these cases): I think we won't have more than a couple in the whole test suite next week.

(this is a response to earlier comment)

tests/test_api.py

The changes are automatic linting fixes from black. The target files are only those who test the new code. Signed-off-by: Martin Vrachev <[email protected]>

All of the changes are made manual. The target files are only those who test the new code. Signed-off-by: Martin Vrachev <[email protected]>

Rename test files testing the old code by adding an "old" suffix. This is done, so we can easily exclude them from linting. Signed-off-by: Martin Vrachev <[email protected]>

Exclude regexs/globs are needed to exclude the test files testing the old code. After we remove those files we will be able to remove the exclude regex/globs. Signed-off-by: Martin Vrachev <[email protected]>

Mypy reported a couple of errors for missing stubs on GitHub https://github.com/theupdateframework/python-tuf/runs/4431787287?check_suite_focus=true The error noted that: "tests/test_api.py:19: error: Library stubs not installed for "dateutil.relativedelta" (or incompatible with Python 3.10)" In order to resolve it I added "types-python-dateutil" into requirements-test.txt Signed-off-by: Martin Vrachev <[email protected]>

Logging configuration is part from each test file begging from commit theupdateframework@03b15fb We should that as well for tests/test_examples.py Signed-off-by: Martin Vrachev <[email protected]>

Make _fetch_metadata and _fetch_taget public by renaming them to fetch_metadata and fetch_target. This will allow the removal of multiple pylint disables because of "accessing private members". Signed-off-by: Martin Vrachev <[email protected]>

Mypy warns us when we assign a not defined variable to an object, but that is something that we are warned for from pylint (seach for "pylint: disable=no-member" in test_updater_key_rotations.py and you will find an example where we have to disable it). We don't want to have two linters checking for the same thing as we can end up disabling two warnings that are actually the same on a single line. Signed-off-by: Martin Vrachev <[email protected]>

Change the delta argument type from the tuf/api/metadata.py module in Signed.bump_expiration() to include relativedelta as this provides an easier interface for the callers. We are already testing for that inside test/api line 338. Signed-off-by: Martin Vrachev <[email protected]>

MVrachev · 2021-12-09T15:04:56Z

I have rebased on top of develop.
It was painful to do so as there were multiple changes and there were new warnings from all of the linters meaning I had to update their commits.
Additionally, I addressed all of Jussi's comments and had to add a couple of new commits (the last two) that handle specific problems.
It would be great if we merge this soon as with each new change it's annoying to do that work again.
@jku and/or @lukpueh can you have a new look?

lukpueh

Many thanks for doing this tedious work, Martin! I'll go forward and merge, because it looks like you addressed Jussi's requested change.

lukpueh · 2021-12-10T09:23:29Z

tests/test_updater_old.py

@@ -21,7 +21,7 @@
  See LICENSE-MIT OR LICENSE for licensing information.

 <Purpose>
-  'test_updater.py' provides a collection of methods that test the public /
+  'test_updater.py_old' provides a collection of methods that test the public /


Is this the coding equivalent of the french Verlan? :P Given the short remaining life time of this doc, I say we leave this as is.

Following parallel merges of theupdateframework#1700 (added new test method), and theupdateframework#1710 (started running mypy on tests), ci/cd fails in the develop branch. This is fixed in this patch. Signed-off-by: Lukas Puehringer <[email protected]>

MVrachev marked this pull request as draft December 6, 2021 14:19

MVrachev force-pushed the start-linting branch 2 times, most recently from 88601b3 to 6f5bd1b Compare December 6, 2021 14:44

lukpueh approved these changes Dec 7, 2021

View reviewed changes

MVrachev force-pushed the start-linting branch from a30a8e8 to 317b9d1 Compare December 7, 2021 10:32

MVrachev marked this pull request as ready for review December 7, 2021 10:35

MVrachev force-pushed the start-linting branch from 317b9d1 to 53a2599 Compare December 7, 2021 10:42

jku mentioned this pull request Dec 8, 2021

Implemented fetch_tracker to RepositorySimulator #1705

Merged

3 tasks

jku requested changes Dec 8, 2021

View reviewed changes

sechkova modified the milestones: Sprint 13, Sprint 14 Dec 8, 2021

MVrachev self-assigned this Dec 8, 2021

MVrachev force-pushed the start-linting branch from b0297b5 to f0bf488 Compare December 9, 2021 13:47

MVrachev added 9 commits December 9, 2021 15:52

Fix more black warnings on test files

a728717

The changes are automatic linting fixes from black. The target files are only those who test the new code. Signed-off-by: Martin Vrachev <[email protected]>

Address more pylint warnings on tests files

8d73330

All of the changes are made manual. The target files are only those who test the new code. Signed-off-by: Martin Vrachev <[email protected]>

Address more mypy warnings on tests files

5aa547d

All of the changes are made manual. The target files are only those who test the new code. Signed-off-by: Martin Vrachev <[email protected]>

Rename old test files by adding old suffix

22fe1e6

Rename test files testing the old code by adding an "old" suffix. This is done, so we can easily exclude them from linting. Signed-off-by: Martin Vrachev <[email protected]>

Start linting test files testing the new code

2f689e7

Exclude regexs/globs are needed to exclude the test files testing the old code. After we remove those files we will be able to remove the exclude regex/globs. Signed-off-by: Martin Vrachev <[email protected]>

Add logging configuration in tests/test_examples

d5d0b94

Logging configuration is part from each test file begging from commit theupdateframework@03b15fb We should that as well for tests/test_examples.py Signed-off-by: Martin Vrachev <[email protected]>

MVrachev force-pushed the start-linting branch from f0bf488 to 811fa27 Compare December 9, 2021 14:46

MVrachev requested a review from jku December 9, 2021 15:06

lukpueh approved these changes Dec 10, 2021

View reviewed changes

lukpueh merged commit 002c828 into theupdateframework:develop Dec 10, 2021

lukpueh mentioned this pull request Dec 10, 2021

test: add missing return type hint in test method #1720

Merged

3 tasks

This was referenced Dec 10, 2021

Test loading of cached metadata in ngclient #1703

Merged

Test delegation graphs #1689

Merged

MVrachev deleted the start-linting branch December 10, 2021 12:00

jku mentioned this pull request Dec 13, 2021

Metadata API: Remove dependency on dateutil #1722

Closed

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

Start linting the test #1710

Start linting the test #1710

MVrachev commented Dec 6, 2021 •

edited

Loading

coveralls commented Dec 6, 2021 •

edited

Loading

lukpueh left a comment

lukpueh Dec 7, 2021

MVrachev Dec 7, 2021

lukpueh Dec 7, 2021

lukpueh Dec 7, 2021

MVrachev Dec 7, 2021

MVrachev Dec 7, 2021

lukpueh Dec 7, 2021

MVrachev Dec 7, 2021

jku Dec 8, 2021

lukpueh commented Dec 7, 2021 •

edited

Loading

MVrachev commented Dec 7, 2021

MVrachev commented Dec 7, 2021

jku left a comment

jku Dec 8, 2021

MVrachev commented Dec 9, 2021

lukpueh left a comment

lukpueh Dec 10, 2021

Start linting the test #1710

Start linting the test #1710

Conversation

MVrachev commented Dec 6, 2021 • edited Loading

coveralls commented Dec 6, 2021 • edited Loading

Pull Request Test Coverage Report for Build 1559295338

💛 - Coveralls

lukpueh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukpueh commented Dec 7, 2021 • edited Loading

MVrachev commented Dec 7, 2021

MVrachev commented Dec 7, 2021

jku left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MVrachev commented Dec 9, 2021

lukpueh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MVrachev commented Dec 6, 2021 •

edited

Loading

coveralls commented Dec 6, 2021 •

edited

Loading

lukpueh commented Dec 7, 2021 •

edited

Loading