Metadata API: comprehensive de/serialization testing

[jku]

It's really hard to avoid de/serialization issues like #1389 and #1386

Can we improve the testing so a large number of different json representatations are deserialized (and then serialized), so that things like missing fields and e.g. empty containers are tested -- I'm not even talking about testing broken data (that would be nice but not first priority), just valid data in all possible forms.

This should be much more doable now that we have the smaller objects: testing the "complete API" of small classes is a much more reasonable goal than testing the whole metadata.

I don't have practical advice right now how this can be implemented without a lot of inline dictionaries...

[joshuagl]

Is this a place where fuzzing could add some confidence that would be harder to achieve with designed test cases? Cc @sechkova

[jku]

pasting a related comment from chat:

the way we currently write the tests makes seeing what is tested very difficult: the test data is constantly built in the middle of the test so you need to read the whole test very carefully to see if a specific case is handled

parameterized testing could help now that we have the smaller classes. There are test dependencies for that but even plain unittest (on python>3.4) includes SubTest that could be used:

testdata = {
    "with attr": "{'attr': 'value'}",
    "missing attr": "{}",
}
def test_data(self):
    for case, data in testdata.items():
        with self.subTest(case=case):
            # deserialize/serialize data here

The value is that A) error message will now include case="missing attr" and B) all cases are tested even on sub-test failure.

An additional decision here is what checks need to be made:

• at least do a round of deserialize + serialize and compare the result
• maybe also compare to an object created with constructor and not deser? This requires more test data

[MVrachev]

I started working on tests for that.
The way I imagined is:

1. The tests will focus on testing truthy cases with valid data.
2. Each test will test deserialization and serialization.
3. Every class should be tested.
4. Every attribute in each class should be tested against a couple of variations of valid data.
   4.1 If there is a limited number of options for an attribute then test with all valid options.
   4.2 If there are no limits on what value an attribute can be (for example 64-character string) then test only for a couple of them.
5. Each time testing an attribute with many valid options, generate random valid data.
   For example to generate 64 character string containing lower case letters and digits:

''.join(random.choice(string.ascii_lowercase + string.digits) for i in range(len))

[jku]

My advice is to not get into detailed tests where different variations of attributes are tested, at least not yet: it's so easy to end up with more and more test code where no-one is really sure what actually gets tested.

I'd like to see readable test cases where I'm able to immediately see e.g. that Targets is tested with

• empty targets dict
• targets dict with data
• no delegations
• delegations

in a way that it's easy to add new cases when we notice that something would be useful

I'm hoping this could be accomplished so that we only define the test data multiple times, and the actual test code is always the same... You actually already do this in at least invalid delegations (EDIT: actually that may have been me). So hypothetical example:

valid_target_cases = {
    "all attributes": TODO-TEST-DATA1,
    "empty targets": TODO-TEST-DATA2,
    "no delegations": TODO-TEST-DATA3,
    # Add things here when new ideas appear
}
def test_serialization_valid_targets(self):
    for case, data in valid_target_cases.items():
        with self.subTest(case=case):
            targets = Targets.from_dict(copy.deepcopy(data))
            self.assertDictEquals(targets.to_dict(), data)
            # maybe other checks?

This is of course worth it (compared to current way of testing) only if the bits with TODO are easy to construct and the whole thing remains readable. It may be worth experimenting with just writing minimal json by hand at least in the simplest classes... but also with generating the data -- maybe using functions written for the purpose, or just by defining fragments that can be re-used in many tests (VALID_DELEGATIONS = {"keys":{VALID_KEYID: VALID_KEY}, "roles":[VALID_ROLE]}) ?

I'm waving my hands wildly as you can see (so no hard opinions here) but I do believe more comprehensive testing requires some level of parameterizing: otherwise the test code becomes a mess no-one wants to touch.

[MVrachev]

I have a couple of questions/observations here:

1. If I create simple tests aiming to test all of the possible valid (de)serialization cases then we have that, but it's spread into different tests. Each time I make an attribute optional I add a test case for that.
   Then, if we want to see clear comprehensive (de)serialization tests we should delete the other tests testing that.
   Examples: 71c4992, 139bfc0 and de2644f.
2. If we are going to move serialization and deserialization separately, I imagine this is better to be in a different test class like TestSerializationDeserialization where we could use local variables to store valid values (as you had mentioned above).
3. Maybe it will be even better if we move those tests in a separate module?

[jku]

What I would like to see in serialization testing is that each component has well defined test sets and minimal amount of code. These test datasets should be easy to read so when I open the file I can quickly check if a case is handled and if not I can easily create a new test case.

There are two main issues with the current code, and these complaints partly apply to the PR here:

• understanding which cases are tested is difficult because test data is read from external files and/or modified in the test code itself
• the test implementation scales badly: Adding a new case either means modifying the external file which is dangerous because it's used by many other tests (modifying it will have unexpected consequences), or adding code to the test function which makes it even more complex and harder to understand

There's probably many ways to address these. I have a quick prototype of DelegatedRole testing (it's three commits on top of this PR: one is just a generic example, one is the helper descriptor, and the last commit contains the actual DelegatedRole test): https://github.com/jku/tuf/commits/test-example

• Uses a decorator to make the test code itself simpler -- now the test literally only needs to deal with one DelegatedRole
• Code only contains three test cases (to match the code it's replacing) but it seems to scale acceptably well to 10 or more cases, and adding a test for invalid cases seems simple as well
• The test function does not need any SetUp(), SetUpClass(), temp directories or to read files: it's self-contained
• It's easy to read the test dataset: compare this to the implementation it replaces which had two bugs in the test data that I could not find before I print-debugged the test. The new test dataset probably has bugs too, but these bugs can be found by just reading the test.
• I did not try to go advanced here but this model also allows more interesting things to be added: maybe we want to include an actual DelegatedRole object in the test dataset along with the json: Assuming our classes had eq() implemented, we could then compare the object instances in the test
• It's obvious some classes serialized form will not fit in one line and in some cases we may want to use test data more real than the one in my example: I would expect some light data generation and using a few lines per test case could still make things acceptable. E.g. in targets test data we could use "keys": {"k1": K1, "k2": K2}, "roles": [DR1, DR2], to fill in some data that we need using constant snippets, still keeping the test dataset readable enough -- so reader can still figure out what a particular test case is testing.

I did not spend a lot of time on this so there's probably weird decisions in the code (does it make sense for the test input to be str? 🤷 does the decorator really need three nested functions? 🤷 how does one actually fill in constant snippets into json since f-strings and json both want to use double quote 🤷), please consider it a prototype