Add ngclient to coverage report #1564
Conversation
Add test_read_timeout to test for urllib3.ReadTimeoutError -> tuf.SlowRetrievalError. Signed-off-by: Teodora Sechkova <tsechkova@vmware.com>
Pull Request Test Coverage Report for Build 1196971876Warning: 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.
Details
💛 - Coveralls |
There was a problem hiding this comment.
I think there are good additions in this pr and the new tests are understandable.
I have only a couple of small suggestions.
I have only one question/suggestion: I see you applied black on test_updater_ng.py.
Have you also enabled linting checks on test_updater_ng.py when calling tox -e lint?
Also, there are other test modules where we haven't applied black and lining - test_apy.py, test_metadata_serialization.py and test_trusted_metadata_set.py (I am not sure for that one).
tests/test_trusted_metadata_set.py
Outdated
| # root is invalid | ||
| root = Metadata.from_bytes(self.metadata["root"]) | ||
| root.signed.version += 1 | ||
| with self.assertRaises(exceptions.RepositoryError): | ||
| test_func(root.to_bytes()) |
There was a problem hiding this comment.
IMO it's not immediately clear why root is valid here, because the exception itself is too general.
Maybe change the comment on line 171 to something like:
root cannot be verified, because signature has changed
There was a problem hiding this comment.
updating the exception to be more specific is an option too
There was a problem hiding this comment.
Updated the exception to UnsignedMetadataError: 61e8f40
tests/test_updater_ng.py
Outdated
| # Launch a SimpleHTTPServer (serves files in the current directory). | ||
| # Test cases will request metadata and target files that have been | ||
| # pre-generated in 'tuf/tests/repository_data', which will be served | ||
| # by the SimpleHTTPServer launched here. The test cases of 'test_updater.py' |
There was a problem hiding this comment.
| # by the SimpleHTTPServer launched here. The test cases of 'test_updater.py' | |
| # by the SimpleHTTPServer launched here. The test cases of 'test_updater._ng.py' |
tests/test_updater_ng.py
Outdated
| self.repository_updater.download_target(targetinfo, self.destination_directory) | ||
|
|
||
| def test_updating_root(self): | ||
| self._modify_repository_root(lambda root: None, bump_version=True) |
There was a problem hiding this comment.
The modification function lambda root: None won't do any changes on root right?
Wanted to make sure we only wanted to test here bumping the version and resigning.
There was a problem hiding this comment.
Yes, I needed a modification function doing nothing.
Modify root tests to cover both loading inital root metadata and updating it. Signed-off-by: Teodora Sechkova <tsechkova@vmware.com>
Add tests covering missing branches of the Updater code. Inlcude ngclient in the total coverage report. Signed-off-by: Teodora Sechkova <tsechkova@vmware.com>
Signed-off-by: Teodora Sechkova <tsechkova@vmware.com>
sechkova
force-pushed
the
improve-coverage
branch
from
609ae15
to
6a178f4
Compare
I applied black mostly because the file was still using the old code style with 2 spaces but it also needed some beautifying. The rest of the test files are much better formatted. I think the common agreement is that we won't apply the code checkers that strictly on the tests (yet), while running black while editing them probably won't hurt if it seems needed. |
|
I modified some comments as suggested in the reviews, you can have another look. |
Fixes #1499, #1552
Description of the changes being introduced by the pull request:
Add ngclient tests for: requests_fetcher.py, trusted_metadata_set.py and updater.py.
Add ngclient to the total coverage report after we've reached 97%.
Please verify and check that the pull request fulfills the following
requirements: