test_keys could use a refactor

[jku]

• Tests are quite long for what they do
• They seemingly test for the same things multiple times
• while also doing things that are not relevant to the documented test case -- maybe testing for some other possible failure?
• initial_setup_for_key_threshold is not just shared initialization, it contains actual tests

I'd much rather see multiple minimal tests for specific purpose.

test_root_has_keys_but_not_snapshot() is an example:

• starts by testing threshold failure twice (once in the initialization function), once in the test itself even though there is a simpler test_snapshot_threshold() that tests for threshold failure already

• the actual test as documented seems to be this:

  repo.root.roles[Snapshot.type].keyids.append(signer.public_key.keyid)
  ...
  # Updating should fail. Root should bump, but not snapshot
  assert client.refresh(init_data) == 1
  

• there's issues here:

  • a new root is never published so the client can never see the modified root
  • adding just a keyid is not correct -- it's fine to test for in one test but here, just like in 99% of cases, you want to add the actual key with root.add_key()
  • there is no way snapshot could bump to anything even if the client was buggy somehow: snapshot remains at the initialized version 3 through the test so the comment makes no sense

It seems like the test as documented could be ~ten lines of code: I suggest using --repository-dump-dir to see what kind of repo you are actually generating.

[AdamKorcz]

When doing this refactoring, I think it is easy to run into the inconsistencies that have surfaced in #86, #87 and #108. IMO it would be great to decide what we do with such inconsistencies in the test suite before refactoring test_keys. I believe our options are:

1. Merge in tests that cause a client to fail. This should probably be our last option.
2. Make the tests optional.
3. Use expected outcome to nullify the failing tests.

[jku]

I think there is only two blockers there:

• adding a keyid to a role without adding the actual key
• adding the same keyid twice into a role

I consider both of these edge cases that should not affect 99% of the test cases.