Tests on the new implementation: address mypy warnings #1687

MVrachev · 2021-11-19T17:00:49Z

Fixes #1657
Related to #1582.

Description of the changes being introduced by the pull request:

This pr includes:

configure mypy to show error codes
fixed type annotations in test_metadata_serialization
two additional invalid serialization tests
manual fixes for a lot of mypy warnings.

When there were mypy warnings that we are calling non-annotated function
in annotated context, I decided to add annotations instead of ignoring
those warnings.
That's how I end up adding annotations in the whole tests/utils.py
module.

NOTE: Those changes are not done by automatic tool, but instead are manual.

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-11-19T17:04:34Z

Pull Request Test Coverage Report for Build 1500766950

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

9 of 9 (100.0%) changed or added relevant lines in 2 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+1.0%) to 98.513%

Totals
Change from base Build 1499283100:	1.0%
Covered Lines:	3799
Relevant Lines:	3825

💛 - Coveralls

jku

Quick comments:

Can you leave comments explaining the ignores -- they don't seem entirely obvious at first glance?
Are requests and dateutil really not annotated anywhere ? I wonder how this has not been an issue so far but now requires ignores...
test_fetcher (and as follow up tuf/requests_fetcher.py) should not need changing as they are part of legacy code

jku · 2021-11-22T09:34:21Z

Can you leave comments explaining the ignores -- they don't seem entirely obvious at first glance?

I mean PR comments, just for review

MVrachev · 2021-11-22T12:52:40Z

tests/test_api.py

        targets_key.verify_signature(md_obj, CanonicalJSONSerializer())
        with self.assertRaises(exceptions.UnsignedMetadataError):
-            targets_key.verify_signature(md_obj, JSONSerializer())
+            targets_key.verify_signature(md_obj, JSONSerializer())  # type: ignore[arg-type]


Here we receive this warning:

tests/test_api.py:194: error: Argument 2 to "verify_signature" of "Key" has incompatible type "JSONSerializer"; expected "Optional[SignedSerializer]" [arg-type]

I disabled it because the test wanted to test setting an explicit set serializer and which will raise
an UnsignedMetadataError and that's what happens.
Please note that JSONSerializer inherits the MetadataSerializer and not the SignedSerializer (as for example the CanonicalJSONSerializer does).
If we replace the JSONSerializer with the CanonicalJSONSerializer the test no longer passes
as we do that same thing on line 192.

isn't this a sign of a bug somewhere then (presumably in the annotations since everything seems to work)?

oh... you mean the test explicitly wants to use a serializer argument that is of a completely different class... but that just happens to have a method of same name serialize().

That's a weird test but not a bug then

MVrachev · 2021-11-22T12:54:59Z

tests/test_api.py

            # Test wrong algorithm format (sslib.FormatError)
            snapshot_metafile.hashes = {
-                256: "8f88e2ba48b412c3843e9bb26e1b6f8fc9e98aceb0fbaa97ba37b4c98717d7ab"
+                256: "8f88e2ba48b412c3843e9bb26e1b6f8fc9e98aceb0fbaa97ba37b4c98717d7ab"  # type: ignore[dict-item]


Here we receive the following mypy warning:

tests/test_api.py:660: error: Dict entry 0 has incompatible type "int": "str"; expected "str": "str" [dict-item]

This error hints we have annotated the snapshot_metafile.hashes as a Dict[str, str], but we
are using it as Dict[int, str].
This is expected behavior here as we purposely test an invalid hash algorithm.

tuf/requests_fetcher.py

MVrachev · 2021-11-22T13:29:10Z

Quick comments:

Can you leave comments explaining the ignores -- they don't seem entirely obvious at first glance?

I left comments about each of them.

Are requests and dateutil really not annotated anywhere ? I wonder how this has not been an issue so far but now requires ignores...

Hmm, I don't remember why I had to add them to the list in setup.cfg. Now when I remove them there aren't any new warnings.
Will remove them.

test_fetcher (and as follow up tuf/requests_fetcher.py) should not need changing as they are part of legacy code

I decided to add type annotations for them as I received warnings that I am calling unannotated code in annotated context.
I don't see it as a problem as those changes are small.

jku · 2021-11-22T13:40:41Z

I decided to add type annotations for them as I received warnings that I am calling unannotated code in annotated context.

I would like to avoid touching any more legacy code at least in this PR than is necessary.

However, I am interested in those warnings -- where do they come from? Why do we end up importing unannotated code that we did not expect to import?

MVrachev · 2021-11-22T14:53:25Z

I removed all annotations added to tuf/request_fetcher.py and the mypy warnings I received are:

tests/test_fetcher.py:61: error: Call to untyped function "RequestsFetcher" in typed context  [no-untyped-call]
tests/test_fetcher.py:75: error: Call to untyped function "fetch" in typed context  [no-untyped-call]
tests/test_fetcher.py:84: error: Call to untyped function "fetch" in typed context  [no-untyped-call]
tests/test_fetcher.py:92: error: Call to untyped function "fetch" in typed context  [no-untyped-call]
tests/test_fetcher.py:101: error: Call to untyped function "fetch" in typed context  [no-untyped-call]
tests/test_fetcher.py:117: error: Call to untyped function "fetch" in typed context  [no-untyped-call]

The problem is that we are testing the request_fetcher inside a test for the newer implementation.
Am I correct to consider test_fetcher.py as a test file for the new code?

MVrachev · 2021-11-22T16:14:28Z

I updated the pr with:

removing ignore_missing_imports from setup.cfg for requests and dateutil
removed changes for tuf/request_fetcher.py and tests/test_fetcher.py as they contain code for the legacy code

jku

Wow, the lack of implicit None return value really makes this a big patch...

Good work: I left some comments but I don't think any are really required fixes. Take a look at the comments but I'm fine with merging as is.

tests/repository_simulator.py

tests/test_api.py

tests/test_fetcher_ng.py

tests/test_updater_with_simulator.py

tests/utils.py

By configuring mypy to show error codes when we get a warning by mypy we will receive an error code as well. Those error codes are useful when you want to disable specific mypy warning for a line with: Signed-off-by: Martin Vrachev <[email protected]>

In test_metadata_serialization.py "test_case_data" is actually a string when the decorator calls the actual test functions. Signed-off-by: Martin Vrachev <[email protected]>

Those tests are about missing "keys" and "roles" attributes in Targets.Delegations. Signed-off-by: Martin Vrachev <[email protected]>

This commit includes manual fixes for a lot of mypy warnings. When there were warnings that we are calling non-annotated function in annotated context I decided to add annotations instead of ignoring those warnings. That's how I end up adding annotations in the whole tests/utils.py module. Signed-off-by: Martin Vrachev <[email protected]>

jku reviewed Nov 22, 2021

View reviewed changes

MVrachev commented Nov 22, 2021

View reviewed changes

tuf/requests_fetcher.py Outdated Show resolved Hide resolved

MVrachev force-pushed the address-mypy-warnings branch from 2a83b1d to b599b92 Compare November 22, 2021 16:09

MVrachev requested a review from jku November 23, 2021 11:16

jku approved these changes Nov 24, 2021

View reviewed changes

MVrachev added 4 commits November 24, 2021 18:29

Configure mypy to show error codes

e18b6ba

By configuring mypy to show error codes when we get a warning by mypy we will receive an error code as well. Those error codes are useful when you want to disable specific mypy warning for a line with: Signed-off-by: Martin Vrachev <[email protected]>

Fix annotations in test_metadata_serialization

1f77b10

In test_metadata_serialization.py "test_case_data" is actually a string when the decorator calls the actual test functions. Signed-off-by: Martin Vrachev <[email protected]>

Add two more invalid serialization tests

0d4d7f8

Those tests are about missing "keys" and "roles" attributes in Targets.Delegations. Signed-off-by: Martin Vrachev <[email protected]>

MVrachev force-pushed the address-mypy-warnings branch from b599b92 to e2deff3 Compare November 24, 2021 18:51

MVrachev requested a review from jku November 24, 2021 18:52

jku merged commit 600eb86 into theupdateframework:develop Nov 25, 2021

MVrachev deleted the address-mypy-warnings branch November 25, 2021 11:25

lukpueh mentioned this pull request Dec 13, 2021

Prepare v0.20.0 #1723

Closed

Tests on the new implementation: address mypy warnings #1687

Tests on the new implementation: address mypy warnings #1687

Uh oh!

Conversation

MVrachev commented Nov 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Nov 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 1500766950

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

Uh oh!

jku left a comment

Choose a reason for hiding this comment

Uh oh!

jku commented Nov 22, 2021

Uh oh!

MVrachev Nov 22, 2021

Choose a reason for hiding this comment

Uh oh!

jku Nov 22, 2021

Choose a reason for hiding this comment

Uh oh!

jku Nov 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MVrachev Nov 22, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MVrachev commented Nov 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jku commented Nov 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MVrachev commented Nov 22, 2021

Uh oh!

MVrachev commented Nov 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jku left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

MVrachev commented Nov 19, 2021 •

edited

Loading

coveralls commented Nov 19, 2021 •

edited

Loading

jku Nov 22, 2021 •

edited

Loading

MVrachev commented Nov 22, 2021 •

edited

Loading

jku commented Nov 22, 2021 •

edited

Loading

MVrachev commented Nov 22, 2021 •

edited

Loading