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 <tsechkova@vmware.com>
Remove parts of the test case which are covered in other tests, this way making its purpose clearer. Signed-off-by: Teodora Sechkova <tsechkova@vmware.com>
Add test cases covering downloading and loading from cache targets with non-matching hash and length. Signed-off-by: Teodora Sechkova <tsechkova@vmware.com>
Use a dataclass for a better representation of the target files in the test data set. Signed-off-by: Teodora Sechkova <tsechkova@vmware.com>
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 |
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: