Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Metadata test full serialization cycle #1860

Merged
merged 1 commit into from
Mar 7, 2022

Conversation

MVrachev
Copy link
Collaborator

Description of the changes being introduced by the pull request:

Replace the usage of Metadata.to_dict inside
test_valid_metadata_serialization and instead use Metadata.to_bytes()
in order to test that the full serialization cycle is working as
expected:
Metadata.from_bytes -> Metadata.to_bytes

Raised from Jussi's comment: #1809 (comment)

Signed-off-by: Martin Vrachev [email protected]

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

Replace the usage of Metadata.to_dict inside
test_valid_metadata_serialization and instead use Metadata.to_bytes()
in order to test that the full serialization cycle is working as
expected:
Metadata.from_bytes -> Metadata.to_bytes

Signed-off-by: Martin Vrachev <[email protected]>
@MVrachev MVrachev force-pushed the serialization-bytes-array branch from c72eb9d to be2c8f0 Compare February 12, 2022 13:46
@coveralls
Copy link

coveralls commented Feb 12, 2022

Pull Request Test Coverage Report for Build 1833914149

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 98.347%

Totals Coverage Status
Change from base Build 1828868126: 0.0%
Covered Lines: 1121
Relevant Lines: 1137

💛 - Coveralls

Copy link
Member

@jku jku left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neither option here is great IMO: we either don't test the full serialization cycle. or we end up hard coding the specific json options in the test (and then the test still won't notice if json module somehow changes how it works).

This isn't a complaint on the patch as such: I'm just saying we need to admit that this type of testing does not work well with complete metadata files and the complete serialization cycle: generating metadata (and verifying the results do not change) is a better approach for that: #1806

Anyway, looks good to me, but let's not try to force a round peg into a square hole any more than this (unless there's some new squaring-a-circle ideas of course)

@jku jku merged commit e7037cf into theupdateframework:develop Mar 7, 2022
@MVrachev MVrachev deleted the serialization-bytes-array branch March 17, 2022 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants