Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add tests for Updater input validation #1758

Merged

Conversation

ivanayov
Copy link
Collaborator

@ivanayov ivanayov commented Jan 5, 2022

This test covers targetinfo, target_path, target_base_url,
metadata_dir and filepath input validation of the Updater
methods

Fixes #1644

  • The code follows the Code Style Guidelines
  • Tests have been added for the bug fix or new feature
  • [n/a] Docs have been added for the bug fix or new feature

@coveralls
Copy link

coveralls commented Jan 5, 2022

Pull Request Test Coverage Report for Build 1778436409

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.

Details

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

Totals Coverage Status
Change from base Build 1777896864: 0.9%
Covered Lines: 3943
Relevant Lines: 3970

💛 - Coveralls

@ivanayov ivanayov force-pushed the updater_api_input_validation branch from 56e6da5 to b754d98 Compare January 6, 2022 09:56
Copy link
Collaborator

@MVrachev MVrachev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I like as a result of this pr is that we can see what exceptions are thrown by different Updater public API functions.
I am planning on fixing the TODO items related to Raises documentation in the public function docstrings from tuf/ngclient/updater.py.

tests/test_updater_validation.py Outdated Show resolved Hide resolved
tests/test_updater_validation.py Outdated Show resolved Hide resolved
tests/test_updater_validation.py Outdated Show resolved Hide resolved
tests/test_updater_validation.py Outdated Show resolved Hide resolved
tests/test_updater_validation.py Outdated Show resolved Hide resolved

updater = self._new_updater_with_targets_setup()
target_info = updater.get_targetinfo("targetpath")
with self.assertRaises(securesystemslib.exceptions.StorageError):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good find, another non-documented Securesystemslib error.. we can handle this in a separate issue though

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ivanayov ivanayov force-pushed the updater_api_input_validation branch from b754d98 to 5479563 Compare January 7, 2022 15:56
Copy link
Member

@jku jku left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've left some specific comments in source.

A general comment on testing any download failures or Fetcher errors: They seem like good new ideas but it might be more useful to add those into test_updater_ng.py as testing this aspect with Repositorysimulator is going to be quite artificial: there's not even a webserver involved...

I think that applies to second half of test_empty_targetinfo_validation and test_invalid_target_base_url.

tests/test_updater_validation.py Outdated Show resolved Hide resolved
tests/test_updater_validation.py Outdated Show resolved Hide resolved
tests/test_updater_validation.py Outdated Show resolved Hide resolved
@ivanayov ivanayov force-pushed the updater_api_input_validation branch from 5479563 to 12466d6 Compare January 12, 2022 11:01
tests/test_updater_ng.py Outdated Show resolved Hide resolved
@ivanayov ivanayov force-pushed the updater_api_input_validation branch from 12466d6 to 7e86e13 Compare January 31, 2022 23:33
@ivanayov
Copy link
Collaborator Author

Linter is failing for examples/repo_example/hashed_bin_delegation.py which is not related to the current changes. I addressed the requirements to verify locally that tox will pass, but didn't commit the change, as it's not related to the PR. This probably came with some linter update, as the file was not changed recently. Can address this in a separate PR.

@MVrachev
Copy link
Collaborator

MVrachev commented Feb 1, 2022

@ivanayov you can rebase your pr on develop.
The pr #1814 which fixes the linting problem was merged.

@ivanayov ivanayov force-pushed the updater_api_input_validation branch from 7e86e13 to 09fffae Compare February 1, 2022 13:13
@ivanayov
Copy link
Collaborator Author

ivanayov commented Feb 1, 2022

@MVrachev thank you for addressing the issue! I just pushed an update.

Copy link
Member

@jku jku left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think two of the tests do not give us much value and could be removed.

Other tests look good... Although we're left with just two short local storage tests in the new file: maybe it's not worth it to add 50 lines of setup code for two tests? We could just move those two to test_updater_ng.py?

tests/test_updater_ng.py Outdated Show resolved Hide resolved
tests/test_updater_validation.py Outdated Show resolved Hide resolved
This test covers `targetinfo`, `target_path`, `target_base_url`,
`metadata_dir` and `filepath` input validation of the `Updater`
methods

Signed-off-by: Ivana Atanasova <iyovcheva@vmware.com>
@ivanayov ivanayov force-pushed the updater_api_input_validation branch from 09fffae to e26363c Compare March 18, 2022 17:12
Copy link
Member

@jku jku left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving, these look good.

I left this question that wasn't responded to though:

...we're left with just two short local storage tests in the new file: maybe it's not worth it to add 50 lines of setup code for two tests? We could just move those two to test_updater_ng.py?

no need to do it now, just wondering about that

@jku jku merged commit b7b035a into theupdateframework:develop Mar 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ngtests: add input validation tests for Updater API
5 participants