New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Extend delegations tests #1711

Merged

sechkova merged 5 commits into theupdateframework:develop from sechkova:test-targetfile-search

Dec 22, 2021

Contributor

sechkova commented Dec 7, 2021 •

edited

Loading

~~WIP since it is built on top of #1689~~

Fixes #1641

Description of the changes being introduced by the pull request:

Extend test_delegation_graphs with tests for:

max number of delegations
invalid metadata
searching for target info

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

This was referenced Dec 8, 2021

Test delegation graphs #1689

Merged

RepositorySimulator: add support for delegated hash bins #1639

Closed

sechkova force-pushed the test-targetfile-search branch from f0a5f63 to 580e206 Compare

December 9, 2021 14:37

coveralls commented Dec 9, 2021 •

edited

Loading

Pull Request Test Coverage Report for Build 1606265815

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

3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+1.0%) to 98.729%

Totals
Change from base Build 1606187413:	1.0%
Covered Lines:	3933
Relevant Lines:	3954

💛 - Coveralls

sechkova force-pushed the test-targetfile-search branch 2 times, most recently from cd02ba2 to 10e6361 Compare

December 15, 2021 10:20

sechkova marked this pull request as ready for review

December 15, 2021 10:22

sechkova force-pushed the test-targetfile-search branch from 10e6361 to 15234b6 Compare

December 17, 2021 14:55

sechkova changed the title ~~WIP: Extend delegations tests~~ Extend delegations tests

sechkova force-pushed the test-targetfile-search branch from 15234b6 to a0e74c1 Compare

December 17, 2021 15:52

jku reviewed

View reviewed changes

Member

jku left a comment

Looks pretty good to me. I'm not sure if my comments are really change requests -- maybe have a look and give your opinion?

tests/test_updater_delegation_graphs.py Outdated

+                              TestDelegation("A", "C"),
+                          ],
+                          # The traversal stops after visiting an invalid role
+                          visited_order=["A"],

Member

jku Dec 20, 2021

could even rename "A" as "invalid" or something to make it clearer what the invalid role is?

tests/test_updater_delegation_graphs.py Outdated

Comment on lines 329 to 333

+                            targets
+                       *.doc / \ r/*/*
+                            A   B
+                          r/x/* / \ r/y/*.zip
+                               C   D

Member

jku Dec 20, 2021

a diagram is not a bad idea but

missing "*.md"
shortening releases to r is not trivial to figure out

Contributor Author

sechkova Dec 20, 2021

True, I'll fix it but now after your comment I'm not sure how long a diagram can remain up to date with the code.

tests/test_updater_delegation_graphs.py Outdated

Comment on lines 358 to 378

+                  targets: utils.DataSet = {
+                      "targetfile": ("targetfile", []),
+                      "README.md": ("README.md", ["A"]),
+                      "releases/x/*": ("releases/x/x_v1", ["B", "C"]),
+                      "releases/y/*.zip": ("releases/y/y_v1.zip", ["B", "D"]),
+                  }

Member

jku Dec 20, 2021

Would it make sense to include boolean for whether the target is found or not? then you could include some negative tests here as well, using the same delegations_tree. Or do you want to keep them separate?

As an example

Add TestTarget("C", b"x release in C", "releases/x/x_v2")
add test case "target exists, not delegated": ("releases/x/x_v2", [ ... ], False)

As another example

Add TestTarget("C", b"duplicate x release in C", "releases/x/x_v1")
existing test case "releases/x/*" now tests that it finds the correct target (and not the one added above). The case name could be improved to explain this

Contributor Author

sechkova Dec 21, 2021

I think extending it for negative cases makes sense but I didn't understand the second example. We add the target files in a dictionary in the simulator so the duplicate will replace the first target?

Member

jku Dec 21, 2021 •

edited

Loading

oh right, if the target path is the same then the url is the same... you are correct my example does not make sense.

tests/test_updater_delegation_graphs.py Outdated

Comment on lines 347 to 350

+                          TestTarget("targets", b"content", "targetfile"),
+                          TestTarget("A", b"content", "README.md"),
+                          TestTarget("C", b"content", "releases/x/x_v1"),
+                          TestTarget("D", b"content", "releases/y/y_v1.zip"),

Member

jku Dec 20, 2021

having the same content is not great if you ever want to check you got the correct file from the correct delegation

tests/test_updater_delegation_graphs.py Outdated

+                          # Confirm that the expected TargetFile is found
+                          assert target is not None
+                          exp_target = self.sim.target_files[targetpath].target_file
+                          self.assertDictEqual(target.to_dict(), exp_target.to_dict())

Member

jku Dec 20, 2021 •

edited

Loading

since content is the same for every test target (see above), this would return true even if we had two different roles serving the same targetpath (and we got the wrong target because of a bug)

Contributor Author

sechkova Dec 21, 2021

So this is pretty much a bug in the test, I added a different content for each test file.

tuf/ngclient/updater.py Outdated

Comment on lines 475 to 478

+                      if (
+                          len(visited_role_names) > self.config.max_delegations
+                          and len(delegations_to_visit) > 0
+                      ):

Member

jku Dec 20, 2021 •

edited

Loading

Suggested change

      
                    if (
          
                        len(visited_role_names) > self.config.max_delegations
          
                        and len(delegations_to_visit) > 0
          
                    ):
          
                    if len(delegations_to_visit) > 0:

I like the simplification you did, and I think this now seems clear... if we have delegations to visit at this point, we must have aborted because max delegations or do I misunderstand?

Contributor Author

sechkova Dec 21, 2021

This is correct, I've missed it.

sechkova added 5 commits

December 20, 2021 19:04


          Modify delegations traversal loop condition

4fc2c19

Reduce the number of variables in the while loop by using
len(visited_role_names) instead of number_of_delegations.
Include equality in the comparison with config.max_delegations
to account for visiting "targets". Shorten the commit message.

Add max number of delegations test case.

Signed-off-by: Teodora Sechkova <tsechkova@vmware.com>


          Add TestTargetFileSearch class

6fa5d3d

Extend test_updater_delegation_graphs.py with tests
for targets metadata search.
- create a new test class TestTargetFileSearch which creates
  a single repository and pefrorms multiple file searches in
  subtests.
- group the common functionality in a base class TestDelegations.
- extend the data classes to accomodate for target_files.

Signed-off-by: Teodora Sechkova <tsechkova@vmware.com>


          Add tests for invalid delegated role metadata

7eea3f9

Extend TestDelegationsGraphs with a test case for
unsigned metadata.

Signed-off-by: Teodora Sechkova <tsechkova@vmware.com>


          Use raw string in TestTargetFileSearch docstirng

d10c8e9

Using a raw string allows the use of backslashes
in the docstring comment whithout them being interpreted
as an escape character.

It also silences pylint W1401: anomalous-backslash-in-string.

Signed-off-by: Teodora Sechkova <tsechkova@vmware.com>


          Add TestTargetCase dataclass

36eaffa

Use a dataclass for a better visual representation of
the test case data set.

Signed-off-by: Teodora Sechkova <tsechkova@vmware.com>

sechkova force-pushed the test-targetfile-search branch from a0e74c1 to 36eaffa Compare

December 21, 2021 10:13

Contributor Author

sechkova commented Dec 21, 2021 •

edited

Loading

Thanks for the suggestions, I find them very reasonable, I did several changes to include them.
There is this one example not clear to me, I've mentioned it in the comment.

sechkova requested a review from jku

December 21, 2021 10:19

jku approved these changes

View reviewed changes

Member

jku left a comment

Yeah, I like it. TestTargetFileSearch is quite complex and requires reader to spend some time to figure it out but I think that's just because this is the most complex thing the client does.

Left a comment about the delegation loop change, please have look. I think your version is fine though.

tuf/ngclient/updater.py

                       # Preorder depth-first traversal of the graph of target delegations.
-                      while number_of_delegations > 0 and len(delegations_to_visit) > 0:
+                      while (
+                          len(visited_role_names) <= self.config.max_delegations

Member

jku Dec 21, 2021

now that I look at this again... doesn't this handle one more delegation than it used to?

The new interpretation could be the correct one (with the assumption that only delegated targets are counted so "targets" itself is not a delegation), just wanted to make sure we're not making a mistake. The test you added for max delegations feels correct to me so I guess I'm ok with this interpretation.

Contributor Author

sechkova Dec 21, 2021

Yeah, in the previous implementation "targets" was counted in the delegations number which I noticed only after adding the test. So I changed a bit the implementation using "<=" here to compensate for targets. I think I mentioned it in the commit message but I can try adding a comment?

Member

jku Dec 21, 2021

ah you did, I just did not understand the connection...

sechkova merged commit ed15d11 into theupdateframework:develop

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet