add test for succint role test case #141
Conversation
AdamKorcz
force-pushed
the
succint-graph-traversal
branch
from
a683fa4 to
80a96ea
Compare
There was a problem hiding this comment.
Looks about right, left some minor comments.
However, I realized only now that the succinct delegations have not made it to the spec yet. I do want this in the test suite but we likely can't have it on by default, that would be annoying to clients who don't implement it.
Sorry to leave another PR hanging but we'll have to figure out how deal with optional features: maybe we add action inputs for them...
| client.download_target(init_data, succinct_bins_graph.target_path) | ||
| # Here we could check that target_info is null. TODO: Add when | ||
| # support for storing the target_info is added. |
There was a problem hiding this comment.
I would check the return value.
I am against support for storing targetinfo
| # Check that metadata dir contains only top-level roles | ||
| assert client._files_exist(TOP_LEVEL_ROLE_NAMES) | ||
|
|
||
| # Looking for a non-existing targetpath forces updater |
There was a problem hiding this comment.
nit: "updater" is a python-tuf component
| # Check that the delegated roles were visited in the expected | ||
| # order and the corresponding metadata files were persisted. | ||
| assert repo.metadata_statistics[2:] == exp_calls | ||
| assert client._files_exist(exp_files) |
There was a problem hiding this comment.
_files_exist() does not verify that only these files exist so I think it adds little value here. I would remove it or wait for #138 where I add a ClientRunner method for that
|
|
||
|
|
||
| @dataclass | ||
| class TestTarget: |
There was a problem hiding this comment.
does this name not make pytest warn?
There was a problem hiding this comment.
PytestCollectionWarning: cannot collect test class 'TestTarget' because it has a __init__ constructor (from: tuf_conformance/test_updater_delegation_graphs.py)
There was a problem hiding this comment.
I'll mark this "changes requested":
- We need a solution for "optional features" before we add this
- optional features must be togglable in the action and in the command line
- default is likely "don't enable"
Since this is not part of the core spec, it's also not that important to work on
Migrates this test from python-tufs test suite: https://github.com/theupdateframework/python-tuf/blob/cc153d71dc607b681b1c2803f21eb58f4526007f/tests/test_updater_delegation_graphs.py#L473
Work on #127