Add Metadata.signatures serialization tests #1809

MVrachev · 2022-01-28T18:10:59Z

Description of the changes being introduced by the pull request:

In a conversation with @jku he pointed out we don't have serialization
tests for Metadata.signatures.
That's how we decided it's worth adding such tests inside
test_metadata_serialization.py and moving the
test case about duplicating signatures inside the same test file.

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

coveralls · 2022-01-28T18:17:17Z

Pull Request Test Coverage Report for Build 1824616067

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.001%) to 98.347%

Totals
Change from base Build 1822645525:	0.001%
Covered Lines:	1121
Relevant Lines:	1137

💛 - Coveralls

jku

"signature" and "metadata" are used in test names in a confusing way if you compare to all existing tests... details in code

tests/test_metadata_serialization.py

MVrachev · 2022-02-10T13:36:53Z

@jku I addressed all of your comments:

I removed the unrecognized_fields test for signatures as it's already tested inside securesystemslib: Add unrecognized fields support for Signature secure-systems-lab/securesystemslib#387
I split the signature and metadata datasets
added real signatures inside the valid metadata set.

jku

You could add a "valid signatures" test set as well (with at least a default case and a case with unrecognised fields). EDIT: oh I see you mentioned that in your comment: I think it wouldn't hurt to have here as well but it's your call.

Left a couple of more suggestions in source, but all of these are probably negotiable...

jku · 2022-02-10T14:05:06Z

tests/test_metadata_serialization.py

+        with self.assertRaises((ValueError, KeyError)):
+            Metadata.from_dict(case_dict)


What do you think about using the full serialization path for Metadata -- Metadata.from_bytes() / Metadata.to_bytes()?

It would be a bit different from other tests in this file -- but feels like a meaningful extension: if we could do a full serialization for other objects too, we would: it's just not possible for those cases.

This applies to the valid test case as well.

jku · 2022-02-10T14:12:31Z

tests/test_metadata_serialization.py

+            "signatures": [ \
+                { \
+                    "keyid": "59a4df8af818e9ed7abe0764c0b47b4240952aa0d179b5b78346c470ac30278d", \
+                    "sig": "5b00100e9cf1c083f8347371ab840cf60124780305124ed7a53fe31bf43473c90b1d2c802ed2f11f5057ba21e6b7a05118b1907f737d2e29c9692aa3345f9801" \
+                }, \
+                { \
+                    "keyid": "8a1c4a3ac2d515dec982ba9910c5fd79b91ae57f625b9cff25d06bf0a61c1758", \
+                    "sig": "de0e16920f87bf5500cc65736488ac17e09788cce808f6a4e85eb9e4e478a312b4c1a2d7723af56f7bfb1df533c67d8c93b6f49d39eabe7fae391a08e1f72f01" \
+                } \
+            ] \


opinions on making test data smaller in this way?

Suggested change

"signatures": [ \

{ \

"keyid": "59a4df8af818e9ed7abe0764c0b47b4240952aa0d179b5b78346c470ac30278d", \

"sig": "5b00100e9cf1c083f8347371ab840cf60124780305124ed7a53fe31bf43473c90b1d2c802ed2f11f5057ba21e6b7a05118b1907f737d2e29c9692aa3345f9801" \

}, \

{ \

"keyid": "8a1c4a3ac2d515dec982ba9910c5fd79b91ae57f625b9cff25d06bf0a61c1758", \

"sig": "de0e16920f87bf5500cc65736488ac17e09788cce808f6a4e85eb9e4e478a312b4c1a2d7723af56f7bfb1df533c67d8c93b6f49d39eabe7fae391a08e1f72f01" \

} \

] \

"signatures": [ { "keyid": "a", "sig": "b" }, { "keyid": "c", "sig": "d" } ] \

I think the massive test data hides what the test does -- it looks like the keyids and signatures are important for the test when they are not. From that perspective the test data should be minimal to test the case that we want to test.

You said you want me to use real signatures?

this is also really valid_metadatas. I think valid_signatures (with actual signatures in it) might be more useful

from #1809 (comment)

so I did... I think I may have been wrong then because A) the serialization does not test the signatures themselves -- just whether the signature object is valid and B) 11 lines for the signatures looks so unreadable

My bad, sorry. Either way works for me.

I prefer if they are shorter, so yea, I will update it.

Wait, I take that back.

I said "valid_signatures (with actual signatures in it) might be more useful". This still sounds reasonable: I probably would not have objected to that.

But your test case here is valid_metadata. Adding 22 lines of unused signature data into the Metadata test (but not doing that in the signature test) is not as useful as it hides what is actually tested

Well, you can have a look at my latest changes and tell me how you finally want it :D.
If it's still doesn't look good I will rever my changes and add the valid signatures inside the signatures test.

Signed-off-by: Martin Vrachev <mvrachev@vmware.com>

Move the duplicating signatures tests from test_metadata_base function in test_api.py into test_metadata_serialization.py. This is a more logical place to store this test case as test_metadata_base is actually focused on testing Metadata.signed.is_expired. That also is the reason why I renamed test_metadata_base to test_metadata_signed_is_expired. Signed-off-by: Martin Vrachev <mvrachev@vmware.com>

There is no need to copy "case_dict" inside serialization test functions in test_metadata_serialization.py when we are testing invalid arguments. These dictionaries are not be used after calling "from_dict" and it doesn't matter if they are empty afterward. Signed-off-by: Martin Vrachev <mvrachev@vmware.com>

MVrachev · 2022-02-10T15:34:11Z

@jku I followed your suggestions.

I also added a new commit updating securesystemslib to 0.22.0 which contains support for unrecognized fields from Signature.
I added that commit so unrecognized fields can be added as a test case for Signature serialization.

jku

I think this is good...

I left a comment on the Metadata serialization tests: like I said I do think there's value in trying to test the full deserialize-serialize cycle (currently the PR never serializes to json).

Doing that would mean the input data would have to be in one of the output styles we support so possibly not as readable (and would be again a little more different from the other serialization tests)
possibly the right choice is to store the Metadata test data in actual static files in git -- admit that full metadata is too big to include in test files themselves

Maybe we just add some details to the existing issue we have for static test data, and accept the metadata tests in this PR for the time being? EDIT I left some thoughts in #1806

jku · 2022-02-11T09:19:31Z

tests/test_metadata_serialization.py

+    def test_valid_metadata_serialization(self, test_case_data: bytes) -> None:
+        md = Metadata.from_bytes(test_case_data)
+        input_dict = json.loads(test_case_data)
+        self.assertDictEqual(input_dict, md.to_dict())


This is better as it runs the whole deserialize-pipeline ... It still doesn't test the whole serialize-pipeline or compare the final result to original input.

I can guess why, you are balancing between keeping the input test data readable and being able to compare the input data to expected output of Metadata.to_bytes().

Did you try if the byte array could be kept made equal to expected output (compact mode or not) and still somewhat readable?

as mentioned in PR comment, I think we can leave this as is but just plan to add better full-cycle serialization tests in near future

I can update the pr later today. If you still think it's okay to compare the data that way then merge it and I can update it in another pr.

jku · 2022-02-11T09:22:25Z

setup.cfg

@@ -37,7 +37,7 @@ packages = find:
 python_requires = ~=3.7
 install_requires =
  requests>=2.19.1
-  securesystemslib>=0.20.0
+  securesystemslib>=0.22.0


hmm, this is probably correct but let's make sure we are doing things for right reasons.

It's true that passing the updated test suite requires this... but this is not really an absolute runtime requirement: python-tuf will work with older securesystemslib just as well as it did before this PR. This PR only changes tests so runtime requirements couldn't possibly have changed: it's a bit weird to imply they did.

On the other hand I don't see an issue with bumping sslib requirement: sslib is practically a part of python-tuf and the bump likely won't make life harder for anyone.

I will remove that to escape controversy and give us an option to decide when to bump it.

PS: I can update the pr later today. If you still think it's okay to bump the securesystemslib version here then go ahead and merge it.

jku

I'll approve but maybe @lukpueh has opinions

lukpueh

Thanks, LGTM!

I'm also fine with bumping sslib runtime requirement. I do see your point Jussi, but I think we can let it go, given that we will require the bump for an upcoming PR and before the next release.

MVrachev requested a review from jku January 28, 2022 18:11

MVrachev force-pushed the signatures-tests branch from 5747673 to c54b8ee Compare January 28, 2022 18:13

jku requested changes Feb 2, 2022

View reviewed changes

tests/test_metadata_serialization.py Outdated Show resolved Hide resolved

tests/test_metadata_serialization.py Outdated Show resolved Hide resolved

tests/test_metadata_serialization.py Show resolved Hide resolved

MVrachev force-pushed the signatures-tests branch 5 times, most recently from a376d06 to ccce48e Compare February 10, 2022 13:35

MVrachev requested a review from jku February 10, 2022 13:37

jku reviewed Feb 10, 2022

View reviewed changes

MVrachev added 4 commits February 10, 2022 17:30

Update securesystemslib version to 0.22.0

bf5c6ee

Signed-off-by: Martin Vrachev <mvrachev@vmware.com>

Add signatures serialization tests

d2a840f

Signed-off-by: Martin Vrachev <mvrachev@vmware.com>

MVrachev force-pushed the signatures-tests branch from ccce48e to 17503eb Compare February 10, 2022 15:30

MVrachev requested a review from jku February 10, 2022 15:34

jku reviewed Feb 11, 2022

View reviewed changes

jku approved these changes Feb 11, 2022

View reviewed changes

lukpueh approved these changes Feb 11, 2022

View reviewed changes

lukpueh merged commit a347d03 into theupdateframework:develop Feb 11, 2022

MVrachev mentioned this pull request Feb 12, 2022

Metadata test full serialization cycle #1860

Merged

3 tasks

MVrachev deleted the signatures-tests branch March 1, 2022 15:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Metadata.signatures serialization tests #1809

Add Metadata.signatures serialization tests #1809

MVrachev commented Jan 28, 2022

coveralls commented Jan 28, 2022 •

edited

Loading

jku left a comment

MVrachev commented Feb 10, 2022

jku left a comment •

edited

Loading

jku Feb 10, 2022

jku Feb 10, 2022

MVrachev Feb 10, 2022 •

edited

Loading

jku Feb 10, 2022

MVrachev Feb 10, 2022

jku Feb 10, 2022 •

edited

Loading

MVrachev Feb 10, 2022

MVrachev commented Feb 10, 2022 •

edited

Loading

jku left a comment •

edited

Loading

jku Feb 11, 2022

jku Feb 11, 2022

MVrachev Feb 11, 2022 •

edited

Loading

jku Feb 11, 2022

MVrachev Feb 11, 2022

MVrachev Feb 11, 2022

jku left a comment

lukpueh left a comment

		with self.assertRaises((ValueError, KeyError)):
		Metadata.from_dict(case_dict)

Add Metadata.signatures serialization tests #1809

Add Metadata.signatures serialization tests #1809

Conversation

MVrachev commented Jan 28, 2022

coveralls commented Jan 28, 2022 • edited Loading

Pull Request Test Coverage Report for Build 1824616067

💛 - Coveralls

jku left a comment

Choose a reason for hiding this comment

MVrachev commented Feb 10, 2022

jku left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MVrachev Feb 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jku Feb 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MVrachev commented Feb 10, 2022 • edited Loading

jku left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MVrachev Feb 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jku left a comment

Choose a reason for hiding this comment

lukpueh left a comment

Choose a reason for hiding this comment

coveralls commented Jan 28, 2022 •

edited

Loading

jku left a comment •

edited

Loading

MVrachev Feb 10, 2022 •

edited

Loading

jku Feb 10, 2022 •

edited

Loading

MVrachev commented Feb 10, 2022 •

edited

Loading

jku left a comment •

edited

Loading

MVrachev Feb 11, 2022 •

edited

Loading