Tests: add tests for non-root key rotations #1691
Conversation
MVrachev
force-pushed
the
key-rotations
branch
from
43d2b8e
to
e5b7a8d
Compare
Pull Request Test Coverage Report for Build 1530822854
💛 - Coveralls |
MVrachev
force-pushed
the
key-rotations
branch
from
e5b7a8d
to
041f8c2
Compare
|
I have rebased on top of develop. |
There was a problem hiding this comment.
I wonder if the for role in roles loop really needs to contain everything it does? Publishing one new root version (that includes the changes for all three metadata) and refreshing once should be enough?
modifying keys, threshold and sigs does need to be in a loop, but nothing else is really required. If the resulting file contents are checked, that needs to happen in a loop as well but I wonder if just checking that the files are there might be enough (assuming subtest_setup removed the files in between)?
tests/test_updater_key_rotations.py
Outdated
| def setUpClass(cls) -> None: | ||
| cls.sim: RepositorySimulator | ||
| cls.metadata_dir: str | ||
| cls.subtest_count = 0 |
There was a problem hiding this comment.
subtest_count doesn't look like class variable, It seems more like a test specific variable to me
Now subtest_count is admittedly a hack that's used to create directory names when dumping repository state (because the subtest name does not seem to be available during the test) so I will also happily take a solution that removes this variable altogether...
One option I considered but did not bother testing was to use str(md_version) (where md_version is the subtests input) as the directory name instead of subtest_count -- that should be a legal directory name on linux at least? If you want to check if that works, I'm fine with trying that
There was a problem hiding this comment.
I have another workaround.
See 51f6baf
tests/test_updater_key_rotations.py
Outdated
| # Call fetch_metadata to sign metadata with new keys | ||
| expected_local_md: Metadata = self.sim._fetch_metadata(role) | ||
| # assert local metadata role is on disk as expected | ||
| md_path = os.path.join(self.metadata_dir, f"{role}.json") | ||
| with open(md_path, "rb") as f: | ||
| data = f.read() | ||
| self.assertEqual(data, expected_local_md) | ||
| # md.version = 1 as only 1 version has been published | ||
| md = Metadata.from_bytes(data) | ||
| self.assertEqual(md.signed.version, 1) |
There was a problem hiding this comment.
I wonder if these checks are useful, or if checking that correct files exist is enough?
There was a problem hiding this comment.
I was wondering the same while I was writing.
The part that is in question for me is the version check as I am not sure if there is a case when the version can end up something different than 1.
The other checks seem okay for me - checking that the local metadata file exists and it's with the content we expect.
Will remove the version check and then we can discuss it again.
There was a problem hiding this comment.
Removed the version check. What do you think about the rest of the checks?
MVrachev
force-pushed
the
key-rotations
branch
2 times, most recently
from
51f6baf
to
78ca6d8
Compare
|
@jku I addressed your comments and the pr is ready for another review. |
RootVersions can be used for key rotation tests beside root and as such it makes sense to rename it to something more generic. Signed-off-by: Martin Vrachev <mvrachev@vmware.com>
Rename expected_result to expected_error as the value expected is never a true result in the current test cases, but it's an expected error. Signed-off-by: Martin Vrachev <mvrachev@vmware.com>
Make setup and teardown classmethods. There is no reason why we would want to create the signers and keys for each test case and this only slows the test module execution. Signed-off-by: Martin Vrachev <mvrachev@vmware.com>
The amount of non-root key rotations is lower than the one for root as the repository stores all root versions in order to be able to do rollback check compared to timestamp, snapshot, and targets where it stores only one version. All of the root roles are needed in those tests again as root delegates the keys for each of the other three top-level metadata roles. Signed-off-by: Martin Vrachev <mvrachev@vmware.com>
Save the case name in the "unittest.TestCase" object when executing test cases through the "run_sub_tests_with_dataset" decorator. This allows dumping directories with names specific to the test case that can be used for debugging as showed in test_updater_key_rotations.py Signed-off-by: Martin Vrachev <mvrachev@vmware.com>
MVrachev
force-pushed
the
key-rotations
branch
from
78ca6d8
to
3def667
Compare
|
Rebased on top of latest develop changes because of a conflict. |
There was a problem hiding this comment.
The test case name trick for dumps is very clever 👏 (I hope others find the --dump feature, it really makes it easier to see what's going on)
The loop in the test makes it a little difficult to figure out what is actually being tested but it does seem to work correctly. I remember we discussed possibly implementing three separate functions for timestamp/snapshot/targets (that would use the same data and maybe same helper function): that would probably be easier to understand ... but maybe it's not worth all that work: this version works just fine.
LGTM, thanks Martin
Fixes #1647
Description of the changes being introduced by the pull request:
The amount of non-root key rotations is lower than the one for root as
the repository stores all root versions in order to be able to do
rollback check compared to timestamp, snapshot, and targets where it
stores only one version.
All of the root roles are needed in those tests again as root
delegates the keys for each of the other three top-level metadata roles.
Additionally, this pr contains some small fixes in the test_updater_key_rotations.py module.
Please verify and check that the pull request fulfills the following
requirements: