Test delegation graphs #1689

sechkova · 2021-11-22T15:35:08Z

Description of the changes being introduced by the pull request:
I added the first tests that came to my mind for traversing the delegations tree/graph. I'd like to receive some feedback on these before continuing further.

For each test/subtest:

create delegations graphs with different complexity
init a new repository with the respective delegations
init a new Updater
look for non-existing target path to force visiting all delegated roles
verify that Updater tried to fetch the expected roles in the expected order
verify that Updater stored the expected metadata files

Please verify and check that the pull request fulfills the following
requirements:

The code follows the Code Style Guidelines
Tests have been added for the bug fix or new feature
Docs have been added for the bug fix or new feature

coveralls · 2021-11-22T15:40:10Z

Pull Request Test Coverage Report for Build 1582213264

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.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

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

Totals
Change from base Build 1582019278:	1.1%
Covered Lines:	3935
Relevant Lines:	3957

💛 - Coveralls

jku

Looks pretty cool, probably a good way to go. It's definitely an improvement over anything that currently exists.

The test data definition is quite chunky, 5-10 lines per subtest, but on the other hand it is readable (at least if test names are better or a comment is added)... and I also can't think of better alternatives: your method allows testing cyclic graphs etc which a dict would have trouble doing.

I would like to be able to run "--dump" to see the actual client side metadata to verify what it looks like. It's not 100% trivial and might look a bit goofy with subtests but can you try to enable that? There's an example in test_updater_key_rotations -- the trick with subtests is that you need to use a new dump directory for each subtest and the directory names are then not great.

All of the comments are really just suggestions: this looks good to me.

jku · 2021-11-23T08:08:28Z

tests/test_updater_delegation_graphs.py

+        keyids: Optional[List[str]] = None,
+        threshold: int = 1,
+        terminating: bool = False,
+        paths: Optional[List[str]] = ["*"],


I think the default should be none, and then you can special case that

if paths is None and path_hash_prefixes is None: paths = ["*"]

otherwise you get bugs when someone actually adds path_hash_prefixes tests

jku · 2021-11-23T08:10:00Z

tests/test_updater_delegation_graphs.py

+@dataclass
+class Delegation:


it's probably not a dataclass if it has a constructor.

the name should maybe change... this is a bit confusing wrt Metadata API

actually the name isn't that bad: I thought it overrode the Metadata API class but that is Delegations... an alternative non-confusing name would be better but the current one works fine.

jku · 2021-11-23T08:21:28Z

tests/test_updater_delegation_graphs.py

+        "3-level tree terminating": {
+            "delegations": [
+                Delegation("targets", "A"),
+                Delegation("targets", "B"),
+                Delegation("A", "C", terminating=True),
+                Delegation("C", "D"),
+            ],
+            "visited roles": ["A", "C", "D"],


these could have a comment pointing out the interesting bit (if it does not fit into the subtest name) -- like I think this is testing that B is not visited because C terminates but I think spelling it out while you still remember could be good...

Alternatively maybe the comment could be the test name?

jku · 2021-11-23T08:57:23Z

tests/test_updater_delegation_graphs.py

+        "single delegation": {
+            "delegations": [Delegation("targets", "A")],
+            "visited roles": ["A"],
+        },


The only tweak I could think of to the test data definition is to define a dataclass for the subtest data as well...

"single_delegation": DelegationTest( delegations = [Delegation("targets", "A")], expect_visited = ["A"] )

but it is just as verbose so not a very useful suggestion. It does get rid of using strings when attributes can work, and would make the types visible in the test code: so you could just use the automatically annotated data.delegations instead having to do delegations: List[Delegation] = data["delegations"]

jku · 2021-11-23T10:44:25Z

look for non-existing target path to force visiting all delegated roles

I'm sure there's loads of tests to be written but at the very least we will want to add tests that actually find the target ... that might require modifying the test data structure as well but starting with this and then adding tests in other PRs sounds good to me

sechkova · 2021-12-01T16:06:09Z

The main improvements are:

the data classes TestDelegation and DelegationsTestCase
tried to improve the sub test names and comments, not sure how much better it is now
added --dump option to all new updater tests

I did modifications at several places so at the end I thought a whole re-review would be better than new commits with the changes.

jku

I think these are cool and something this project has never had. With merge conflict fixed LGTM.

Your sub test cleanup commit implies we should have a proper solution for that? The subtest model seems to work really well for our use cases but the manual cleanup is really not optimal...

These tests only cover the "no search results case" but I suppose the same setup (maybe even same test data?) can be used for the positive case in follow up PRs?

jku · 2021-12-08T08:43:01Z

tests/test_updater_delegation_graphs.py

+            # unpack 'd' but skip "delegator"
+            role = DelegatedRole(*astuple(d)[1:])


oh wow. mypy is going to have an aneurysm I bet :)

Maybe it was paralysed by the shock because it stayed silent when I tried 😁

jku · 2021-12-08T08:47:52Z

tests/test_updater_delegation_graphs.py

+            with patch.object(
+                sim, "_fetch_metadata", wraps=sim._fetch_metadata
+            ) as wrapped_fetch:


Once the fetcher statistics PR is in we should modify this to use them ... but it's not in quite yet

sechkova · 2021-12-08T09:18:42Z

Your sub test cleanup commit implies we should have a proper solution for that? The subtest model seems to work really well for our use cases but the manual cleanup is really not optimal...

I agree, I will open an issue for improving it, I haven't figured out a clever solution for now.

These tests only cover the "no search results case" but I suppose the same setup (maybe even same test data?) can be used for the positive case in follow up PRs?

The follow up PR: #1711

jku · 2021-12-09T12:10:26Z

I agree, I will open an issue for improving it, I haven't figured out a clever solution for now.

I guess it would have to integrate with the subtest runner somehow: maybe we could make class SubTestCase(unittest.TestCase) that contains the subtest decorator as class method (if that is even possible) and then the decorator could handle calling cls.setup_subtest() and cls.teardown_subtest()?

I guess the adult way of solving this is to use some test library that does this for us 🤷 but where's the fun in that?

sechkova · 2021-12-09T14:14:27Z

The conflicts are resolved, I added one extra commit for using ReposiotrySimulator.fetch_tracker.

sechkova · 2021-12-09T14:14:47Z

I agree, I will open an issue for improving it, I haven't figured out a clever solution for now.

I guess it would have to integrate with the subtest runner somehow: maybe we could make class SubTestCase(unittest.TestCase) that contains the subtest decorator as class method (if that is even possible) and then the decorator could handle calling cls.setup_subtest() and cls.teardown_subtest()?

I guess the adult way of solving this is to use some test library that does this for us shrug but where's the fun in that?

Here is the issue: #1719

MVrachev · 2021-12-10T10:48:27Z

Could you please rebase on top of develop?
In #1710 we started linting our tests and it will be good to lint the changes made in this pr.

Make the method _cleanup_dir public and move it to tests/utils.py. Signed-off-by: Teodora Sechkova <[email protected]>

Reduce the number of function arguments and use DelegatedRole instead. When adding a list of delegations to the repository, move the Targets creation inside the loop to create a separate Targets object for each delegation. Create a new Metadata obgect only for delegated roles which do not exist yet in the repository. Signed-off-by: Teodora Sechkova <[email protected]>

Use a "try" block to catch exceptions during failing subtests and always execute the subtest clean up code. Signed-off-by: Teodora Sechkova <[email protected]>

sechkova · 2021-12-10T14:22:02Z

Several "rebases" happened after the last review. Should be ok.

jku

I'm good to go with this as a start. Left some minor comments, please have a look and decide for yourself.

We do want more delegation tests in future. Some obvious ones:

delegation paths that actually match the targetpath (current test never has any hits)
delegation paths with directories (and wildcards in directories)
path_hash_prefixes

jku · 2021-12-13T10:31:02Z

tests/test_updater_top_level_update.py

    def tearDown(self) -> None:
+        if self.dump_dir is not None:
+            self.sim.write()


Minor nit: dumping here and not when client refreshes means that you only get the end state -- some of the tests do have multiple repository states (like test_new_timestamp_snapshot_rollback) and those are not visible like this.

Yes, that's right, I moved it to _run_refresh and _init_updater: 1ba9301

jku · 2021-12-15T08:32:40Z

tests/test_updater_delegation_graphs.py

+    keyids: List[str] = field(default_factory=list)
+    threshold: int = 1
+    terminating: bool = False
+    paths: List[str] = field(default_factory=lambda: ["*"])


nit: when you want to actually test path_hash_prefixes this needs to be optional... but we can fix that when it happens

Add tests creating delegations graphs with different complexity and successfully updating the delegated roles metadata. Signed-off-by: Teodora Sechkova <[email protected]>

Extend updater tests with the option to dump repository metadata locally. Signed-off-by: Teodora Sechkova <[email protected]>

Replace the use of "patch" in test_graph_traversal with the newly added fetch_tracker from RepositorySimulator. Signed-off-by: Teodora Sechkova <[email protected]>

Signed-off-by: Teodora Sechkova <[email protected]>

sechkova · 2021-12-15T10:26:26Z

We do want more delegation tests in future. Some obvious ones:
* delegation paths that actually match the targetpath (current test never has any hits)

This is done in the follow up PR #1711

* delegation paths with directories (and wildcards in directories)

Yeah, this can be added I guess here #1711

* path_hash_prefixes

Already an issue about it: #1639 (comment)

jku reviewed Nov 23, 2021

View reviewed changes

sechkova force-pushed the test_delegated_roles branch from 85dcbc8 to dc908ee Compare December 1, 2021 15:53

sechkova requested a review from jku December 1, 2021 16:06

MVrachev mentioned this pull request Dec 2, 2021

Sanitize repo simulator role names when writing #1702

Merged

3 tasks

sechkova mentioned this pull request Dec 7, 2021

Extend delegations tests #1711

Merged

3 tasks

jku approved these changes Dec 8, 2021

View reviewed changes

sechkova mentioned this pull request Dec 8, 2021

RepositorySimulator: add support for delegated hash bins #1639

Closed

sechkova force-pushed the test_delegated_roles branch from dc908ee to ff8e5a3 Compare December 9, 2021 13:49

sechkova mentioned this pull request Dec 9, 2021

Improve subtests clean up #1719

Open

sechkova added 3 commits December 10, 2021 15:59

tests: move _cleanup_dir to utils.py

5685807

Make the method _cleanup_dir public and move it to tests/utils.py. Signed-off-by: Teodora Sechkova <[email protected]>

Improve clean up in subtests

74dddf0

Use a "try" block to catch exceptions during failing subtests and always execute the subtest clean up code. Signed-off-by: Teodora Sechkova <[email protected]>

sechkova force-pushed the test_delegated_roles branch from ff8e5a3 to c30933b Compare December 10, 2021 14:15

jku approved these changes Dec 15, 2021

View reviewed changes

sechkova added 4 commits December 15, 2021 12:12

Add test_updater_deleagation_graphs.py

db0e0d6

Add tests creating delegations graphs with different complexity and successfully updating the delegated roles metadata. Signed-off-by: Teodora Sechkova <[email protected]>

Add --dump option to tests

1ba9301

Extend updater tests with the option to dump repository metadata locally. Signed-off-by: Teodora Sechkova <[email protected]>

Use fetch_tracker in test_graph_traversal

26a5f81

Replace the use of "patch" in test_graph_traversal with the newly added fetch_tracker from RepositorySimulator. Signed-off-by: Teodora Sechkova <[email protected]>

Fix linter errors in tests

2562aff

Signed-off-by: Teodora Sechkova <[email protected]>

sechkova force-pushed the test_delegated_roles branch from c30933b to 2562aff Compare December 15, 2021 10:15

jku merged commit 0f1fc6e into theupdateframework:develop Dec 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test delegation graphs #1689

Test delegation graphs #1689

sechkova commented Nov 22, 2021

coveralls commented Nov 22, 2021 •

edited

Loading

jku left a comment

jku Nov 23, 2021

jku Nov 23, 2021

jku Nov 30, 2021 •

edited

Loading

jku Nov 23, 2021

jku Nov 23, 2021

jku commented Nov 23, 2021 •

edited

Loading

sechkova commented Dec 1, 2021

jku left a comment

jku Dec 8, 2021

sechkova Dec 8, 2021

jku Dec 8, 2021

sechkova commented Dec 8, 2021

jku commented Dec 9, 2021

sechkova commented Dec 9, 2021

sechkova commented Dec 9, 2021

MVrachev commented Dec 10, 2021

sechkova commented Dec 10, 2021

jku left a comment

jku Dec 13, 2021

sechkova Dec 15, 2021

jku Dec 15, 2021 •

edited

Loading

sechkova commented Dec 15, 2021

		# unpack 'd' but skip "delegator"
		role = DelegatedRole(*astuple(d)[1:])

Test delegation graphs #1689

Test delegation graphs #1689

Conversation

sechkova commented Nov 22, 2021

coveralls commented Nov 22, 2021 • edited Loading

Pull Request Test Coverage Report for Build 1582213264

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

jku left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jku Nov 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jku commented Nov 23, 2021 • edited Loading

sechkova commented Dec 1, 2021

jku left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sechkova commented Dec 8, 2021

jku commented Dec 9, 2021

sechkova commented Dec 9, 2021

sechkova commented Dec 9, 2021

MVrachev commented Dec 10, 2021

sechkova commented Dec 10, 2021

jku left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jku Dec 15, 2021 • edited Loading

Choose a reason for hiding this comment

sechkova commented Dec 15, 2021

coveralls commented Nov 22, 2021 •

edited

Loading

jku Nov 30, 2021 •

edited

Loading

jku commented Nov 23, 2021 •

edited

Loading

jku Dec 15, 2021 •

edited

Loading