Skip to content
This repository was archived by the owner on Aug 18, 2020. It is now read-only.

{DO NOT MERGE} [CDEC-512] Create JSON round-trip and golden tests for GenesisData, GenesisProtocolConstants, and GenesisConfiguration #3458

Closed
wants to merge 5 commits into from

Conversation

intricate
Copy link
Contributor

@intricate intricate commented Aug 23, 2018

Description

We have to create and expand upon existing round-trip and golden tests for the aforementioned data types in order to ensure that the upcoming implementation of CO-354 doesn't break configuration backwards compatibility.

Linked issue

https://iohk.myjetbrains.com/youtrack/issue/CDEC-512

Type of change

  • 🐞 Bug fix (non-breaking change which fixes an issue)
  • 🛠 New feature (non-breaking change which adds functionality)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)
  • 🏭 Refactoring that does not change existing functionality but does improve things like code readability, structure etc
  • 🔨 New or improved tests for existing code
  • ⛑ git-flow chore (backport, hotfix, etc)

Developer checklist

  • I have read the style guide document, and my code follows the code style of this project.
  • If my code deals with exceptions, it follows the guidelines.
  • I have updated any documentation accordingly, if needed. Documentation changes can be reflected in opening a PR on cardanodocs.com, amending the inline Haddock comments, any relevant README file or one of the document listed in the docs directory.

Testing checklist

  • I have added tests to cover my changes.
  • All new and existing tests passed.

QA Steps

Screenshots (if available)

@intricate intricate added the wip label Aug 23, 2018
@intricate intricate self-assigned this Aug 23, 2018
@intricate intricate force-pushed the intricate/CDEC-512 branch 4 times, most recently from 58f2229 to 88198d6 Compare August 23, 2018 03:01
@intricate intricate requested a review from ruhatch August 23, 2018 03:02
@intricate intricate removed the wip label Aug 23, 2018
Copy link
Contributor

@Jimbo4350 Jimbo4350 left a comment

Choose a reason for hiding this comment

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

Just a question about encoding/decoding.

@@ -38,6 +43,35 @@ goldenTestJSON x path = withFrozenCallStack $ do
Left err -> failWith Nothing $ "could not decode: " <> show err
Right x' -> x === x'

-- | Only check that the datatype equals the decoding of the file
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we also be checking the encoding?
#3415

Copy link
Contributor

Choose a reason for hiding this comment

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

The reason for these functions is to ensure backwards compatibility with existing serialized datatstructures, with default values for missing fields. We want to demonstrate that when the PR is merged, the tests for “deserialize this bytestring and check that the value is equal to a target value” still pass (once the given datatype is modified, in the source code, to have the default value in the newly added record field)

=> a
-> FilePath
-> Property
goldenTestCanonicalJSONDec x path = withFrozenCallStack $ do
Copy link
Contributor

Choose a reason for hiding this comment

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

Same question as above.

Copy link
Contributor

Choose a reason for hiding this comment

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

See above comment. These should be used sparingly, only to ensure backwards compatibility.

intricate and others added 5 commits August 23, 2018 12:09
The To/FromJSON instances for Microsecond store the value as an
integer denoting the # of Seconds. So they are rounded to the nearest
10^6.
Luke hit some weird type errors and I got them to go away by adding
a type signature.

This commit uses `roundTripsCanonicalJSONShow` to test `GenesisData`.
Jimbo4350
Jimbo4350 previously approved these changes Aug 23, 2018
@Jimbo4350 Jimbo4350 dismissed their stale review August 24, 2018 13:20

Conflicting files on present on this branch.

Copy link
Contributor

@ruhatch ruhatch left a comment

Choose a reason for hiding this comment

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

LGTM!

@mhuesch mhuesch changed the title [CDEC-512] Create JSON round-trip and golden tests for GenesisData, GenesisProtocolConstants, and GenesisConfiguration {DO NOT MERGE} [CDEC-512] Create JSON round-trip and golden tests for GenesisData, GenesisProtocolConstants, and GenesisConfiguration Aug 28, 2018
@mhuesch
Copy link
Contributor

mhuesch commented Sep 7, 2018

Closing this, as we chose a different approach for CO-354.

@mhuesch mhuesch closed this Sep 7, 2018
@intricate intricate deleted the intricate/CDEC-512 branch October 8, 2018 12:53
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants