ngtests: Test fetch target #1728
Conversation
Pull Request Test Coverage Report for Build 1606548701
💛 - Coveralls |
There was a problem hiding this comment.
Looks good. I think the cache-miss case should test that we successfully overwrite the "incorrect" file as well. Other comments are just nits.
It feels like three test cases is not a lot... but I can't think of what else to add so 🤷
Would using delegated targets give us any useful test coverage? the actual delegation lookups are tested elsewhere so maybe not?
tests/test_updater_fetch_target.py
Outdated
| self.temp_dir.cleanup() | ||
|
|
||
| def _init_updater(self) -> Updater: | ||
| """Creates a new updater and runs refresh.""" |
tests/test_updater_fetch_target.py
Outdated
| return updater | ||
|
|
||
| targets: utils.DataSet = { | ||
| "standard case": ("targetpath", b"content", "targetpath"), |
There was a problem hiding this comment.
I think it would be good to add at least a comment about the contents of this tuple (or make it into a namedtuple or dataclass). Understanding what each "targetpath" means currently requires reading code.
There was a problem hiding this comment.
I added a data class for target files in another PR before this one and I was planning to use it but it is not yet merged so in other words I got tangled in my own PRs ...
There was a problem hiding this comment.
I added a dataclass TestTarget in test_updater_fetch_target.py too which is slightly different than the one in test_updater_delegation_graphs.py . I realise the same name is not great but I was wondering should I rename one of the classes or we should try making the dataclasses reusable between test files. d1bc201
tests/test_updater_fetch_target.py
Outdated
| # Newer content is detected, old cached version is not used | ||
| updater = self._init_updater() | ||
| info = updater.get_targetinfo(targetpath) | ||
| assert info is not None | ||
| self.assertIsNone(updater.find_cached_target(info)) |
There was a problem hiding this comment.
please continue with download_target and verifying that
- find_cached_target() now returns the path
- cached files now has the new content
Add a new test file and class for testing target files fetching. Move test_targets from test_updater_with_simulator.py to tests/test_updater_fetch_targets.py. Signed-off-by: Teodora Sechkova <[email protected]>
Remove parts of the test case which are covered in other tests, this way making its purpose clearer. Signed-off-by: Teodora Sechkova <[email protected]>
Add test cases covering downloading and loading from cache targets with non-matching hash and length. Signed-off-by: Teodora Sechkova <[email protected]>
Use a dataclass for a better representation of the target files in the test data set. Signed-off-by: Teodora Sechkova <[email protected]>
sechkova
force-pushed
the
test-fetch-target
branch
from
7a902dc to
d1bc201
Compare
|
The cache miss test is improved and I agree defining a dataclass does improve readability I am not sure about the naming though, see my comment above. |
|
LGTM |
Pull Request Test Coverage Report for Build 1606548701Warning: 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 |
Fixes #1642
Description of the changes being introduced by the pull request:
Adds tests for 'Fetch target' from 'Detailed client workflow' and target files storing/loading from cache.
Please verify and check that the pull request fulfills the following
requirements: