Add test coverage for delegated hash bins

[ivanayov]

This change adds tests coverage for path_hash_prefixes and
verifies that role names matching specific prefixed successfully
find and download the corresponding metadata files

Fixes #1639

• The code follows the Code Style Guidelines
• Tests have been added for the bug fix or new feature
• [n/a] Docs have been added for the bug fix or new feature

[jku]

marking request changes so it's visible on PR list: we talked about this over zoom, and found two issues: ivana promised a refactor at some point

[coveralls]

Pull Request Test Coverage Report for Build 1878475404

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 (+0.2%) to 98.573%

---

Totals
Change from base Build 1875649035:	0.2%
Covered Lines:	1094
Relevant Lines:	1106

---

💛 - Coveralls

[jku]

There are still some problems here:

1. roles_pref_map still seems unnecessary: _init_repo() call already adds the delegations defined in your test data so there seems to be no reason to then add them manually? If the data driven test setup (defining a Dataset outside the test) does not work in this case for some reason, then let's not use it: defining a Dataset outside of the test and then also generating the test data in the test code combines the worst of both worlds.
2. test comments say we verify that no metadata file was downloaded as expected for non-existing path, however:
   • we do expect delegated metadata to be downloaded for non-existing targetpaths if and only if the prefix of the hash of the targetpath matches the one in the delegation: this what the test is trying to test
   • test does not verify whether metadata was downloaded or not: it should
3. The tests in TestDelegationsGraphs are so far concentrated on testing delegations only: TargetFiles are not needed. I think this is a good idea for your test as well as it makes the test data smaller and focuses the test better. Yes, we should add target download tests for hash prefixes case but it does not need to happen in this class and it does not need to happen in this PR.

The test should assert the same things test_graph_traversal() does for the "paths" case: assert that after get_targetinfo()
correct metadata files are in cache and that the expected repository requests were made (see how visited_order is used to check both of those).

The complication with this is that while DelegationsTestCase defines visited_order as the expected delegated targets that are downloaded, it does not define the targetpath that is used: the targetpath is currently hard coded in the test, usually as "missingpath". I'm totally fine doing the same in this test for the first version but I do expect you to assert that visited_order is correct: currently it is not used at all.

---

This is outside of scope for this PR but in case it makes the intent clearer: What DelegationsTestCase maybe should do is define a dict of target_paths and visited_orders:

search_results: {
    "missingpath": ["A", "B", "C"]
    "noMatchInCPaths": ["A", "B"]
}

in your path_hash_prefixes cases the search results would look like

search_results: {
    "missingpath": ["someprefix"]
    "othermissingpath": ["otherprefix"]
}

because with your test data you will only get a single delegated role for every search.

[ivanayov]

Thank you for the review @jku ! I changed the test as you proposed and moved the old with target downloads to #1910 where your comments are addressed as well.

[jku]

Thank you, this looks much better. I have two issues left but I'm starting to think they may be for follow up issues:

• The purpose of TestDelegation was to make the test case definition short. These are now ridiculously long, 14 lines per delegation or 50 lines for a single test case.
• The test does not actually check that target lookups lead to the correct metadata file downloads, it only checks that after three lookups, all three are downloaded (so we could e.g. have a bug where we download all three on the first lookup, and would not notice that)

I think we could leave those for later PRs... but I did make a suggestion on how to make the TestDelegation shorter in lines (it could also cover all hashes and not just a small subset). Let me know if you'd like to still handle that here or not.

[jku]

Oh yeah, much more readable. Thanks

Please fix the delegation role names, they don't currently make any sense (this does not affect the test functionality but I think the current names mislead the reader). This should trigger the the CI as well