Test loading of cached metadata in ngclient

[ivanayov]

After making a successful update of valid metadata which stores it
in cache and performing a second update with a new updater while
the metadata is already stored in cache, this test verifies that
timestamp, snaphot and targets are loaded from cache and not
downloaded

Addresses #1681

Signed-off-by: Ivana Atanasova iyovcheva@vmware.com

• 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

[coveralls]

Pull Request Test Coverage Report for Build 1653759271

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.0%) to 98.729%

---

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

---

💛 - Coveralls

[kairoaraujo]

Hi @ivanayov, I think my PR #1704 will help you to remove the mock part. You will be able to test it directly against the Repository Simulator.

[ivanayov]

@jku I addressed your comment, if something is not very correct I'll edit it. (It doesn't cover if the loaded timestamp is used to verify the new timestamp, but AFAIR it's already covered and we wanted to check only what's loaded from disk.)

Thanks @kairoaraujo . With the latest change I don't check fetch anymore, but will keep in mind for future tests.

[MVrachev]

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.

[jku]

Yes, this looks ok. Left some comments on get_targetinfo() calls: please fix those.

Since you are already modifying I'll leave another nitpick: can you double check that comments and commit messages do not claim to do something that isn't done: at least one comment says "Test that timestamp, snaphot and targets are ... not downloaded" which is not true. The test only checks that local files are opened in the expected way (and that's fine, this is useful and would have caught a past issue we had).

There is a potential follow-up for

• adding a delegated target to repository (se the test can check that delegated target is also correctly loaded from local disk)
• separately testing constructor, refresh() and get_targetinfo()

[sechkova]

I added some comments to the code but then realized, this test does not prove much. Updater always tries to load metadata from cache. There will always be calls to open even if there are no cached files. If this is what we want to demonstrate, ok. Then we are testing that "Updater always tries to read the cache before downloading metadata" and a single call to "refresh" is enough. The whole test would be:

updater = self._run_refresh()
wrapped_open.assert_has_calls([
            call(os.path.join(self.metadata_dir, "root.json"), "rb"),
            call(os.path.join(self.metadata_dir, "timestamp.json"), "rb"),
            call(os.path.join(self.metadata_dir, "snapshot.json"), "rb"),
            call(os.path.join(self.metadata_dir, "targets.json"), "rb"),
        ])

Probably a way to show that it opened and used the cached metadata is to show that when cached metadata is loaded from the file system successfully, there are no calls to RepositorySimulator.fetch_metadata for snapshot and targets. This should be easier now, after the addition of RepositorySimulator.fetch_counters. "Usage" of local (intermediate) root is described in the spec and should have be covered in test_top_level_update.py. Timestamp is a special case that can be tested in another set of tests covering invalid metadata.

The test is not really consistent. It randomly calls _run_refresh and get_targetinfo.
In case it is not clear, the options are either:

1. Test only top-level roles metadata

• create a new updater, call updater.refresh to get the metadata locally
• reset the mock
• create a new updater, call updater.refresh a second time to get the metadata loaded from the local cache dir
• check the calls to open, check the calls to RepositorySimulator.fetch_tracker.metadata

Then as Jussi suggests, add delegations as follow up.

2. Test both top-level metadata and a delegated role

• add a delegated role to simulator, there is an example in

  python-tuf/tests/test_updater_with_simulator.py

  Line 148 in bdf1cbb

  self.sim.add_delegation(

• create a new updater, call updater.get_targetinfo("non_existent_target") to get the metadata locally, delegated role included
• reset the mock
• create a new updater, call updater.get_targetinfo("non_existent_target") a second time to get the metadata loaded from the local cache dir
• check the the calls to open, check the calls to RepositorySimulator.fetch_tracker.metadata

[sechkova]

The test makes sense to me now, thanks for updating it. Some minor comments still but it looks ok to me.

[jku]

LGTM: @sechkova all yours to merge when you're happy

[ivanayov]

Latest force-push is with addressing @sechkova 's comment, nothing else has changed.

[sechkova]

@jku GH seems to believe you're still requesting changes and I cannot merge without your approval.

[jku]

Sorry Ivana, I just made your life a bit harder by merging Teodoras test move.

Could you rebase?

(test looks really nice btw)

[ivanayov]

@jku it's fine, I migrated it to tests/test_updater_top_level_update.py