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

[CDEC-506] Make JSON golden tests check FromJSON decoding in addition to ToJSON encoding #3415

Merged
merged 2 commits into from
Aug 16, 2018
Merged

[CDEC-506] Make JSON golden tests check FromJSON decoding in addition to ToJSON encoding #3415

merged 2 commits into from
Aug 16, 2018

Conversation

mhuesch
Copy link
Contributor

@mhuesch mhuesch commented Aug 15, 2018

Description

It recently came to my attention that our goldenTestJSON helper function only checks that the provided datastructure's JSON encoding is equal to the ByteString in the file. It doesn't check that the decoding of the ByteString is a datastructure which is == to the provided datastructure.

By comparison goldenTestBi checks both ways. Compare to: goldenTestJSON

This means we're missing about half of the test coverage we want on all JSON golden tests.

These 2 commits fix these issues.

Linked issue

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

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.
  • CHANGELOG entry has been added and is linked to the correct PR on GitHub.
    ^ minor fix - will record in a YouTrack comment, and it can be mentioned in the parent CHANGELOG entry.

Testing checklist

  • I have added tests to cover my changes.
  • All new and existing tests passed.
    ^ existing tests should cover this bugfix. I fixed up one test.

@mhuesch
[CDEC-506] goldenTestJSON: check FromJSON decoding
b17aa39 
Make JSON golden tests check `FromJSON` decoding in addition to
`ToJSON` encoding.
@mhuesch mhuesch self-assigned this Aug 15, 2018
Jimbo4350
Jimbo4350 previously approved these changes Aug 15, 2018
View reviewed changes
@Jimbo4350 Jimbo4350 dismissed their stale review August 15, 2018 20:46

CI hasn't finished

erikd
erikd reviewed Aug 15, 2018
View reviewed changes
core/test/Test/Pos/Core/ExampleHelpers.hs Outdated
"\22940\29914\144828\177644\52019\16682\5563\142278\22269\41464\
\\64899\153034\134491\51443\9384\49583\41181\16586\45161\55231\
\\127978\3521\6732\67841\152627\1796\53978\28040\68524\45059\
\\46383\4326"
Copy link
Member

Choose a reason for hiding this comment

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

We have hex encoding\decoding helpers somewhere. Is i worth using them for this? IMO hex is much more readable and uniform than this stuff.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean the Base16 stuff? Yeah, that might make this more readable (although these are just random Char's I generated).

Copy link
Member

Choose a reason for hiding this comment

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

Hex is more readable than what you have :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just force-pushed a new version which uses Serokell.Util.Base16 to encode the ByteStrings. Let me know if that's more like what you imagined.

erikd
erikd reviewed Aug 15, 2018
View reviewed changes
core/test/golden/GenesisConfiguration_GCSpec
{"spec":{"avvmDistr":{"nNq87DMqu8b9-IPKW_Oor93Kab_qwUwBMwTaiKwDL-Y=":37343863242999412,"4qF3OiqC0QwwiQy_hOzL3Bqq7pIElkJNNuhoA52ctRk=":36524597913081152},"ftsSeed":"RTVTNGZTSDZldE5vdWlYZXpDeUVqS2MzdEc0amEwa0Y=","heavyDelegation":{"ed41bf35f4cd2109bdee27b0439942f6de3f55f0ac8d170de7136d3d":{"pskDelegatePk":"3cppv+rBTAEzBNqIrAMu5jKBqwNsGxuRiOSxdLMD9D5VFjsXjpmbn9UGN7Ltq4yFioeaw8S9PmEAlUGaGWllcw==","pskOmega":68300481033,"pskCert":"bae5422af5405e3803154a4ad986da5d14cf624d6701c5c78a79ec73777f74e13973af83752114d9f18166085997fc81e432cab7fee99a275d8bf138ad04e103","pskIssuerPk":"4qF3OiqC0QwwiQy/hOzL3Bqq7pIElkJNNuhoA52ctRkhsl7+Az2bANTwLM2c2rzsMyq7xv34g8pb86iv9KrCfg=="}},"blockVersionData":{"scriptVersion":999,"slotDuration":999,"maxBlockSize":999,"maxHeaderSize":999,"maxTxSize":999,"maxProposalSize":999,"mpcThd":9.9e-14,"heavyDelThd":9.9e-14,"updateVoteThd":9.9e-14,"updateProposalThd":9.9e-14,"updateImplicit":99,"softforkRule":{"initThd":9.9e-14,"minThd":9.9e-14,"thdDecrement":9.9e-14},"txFeePolicy":{"txSizeLinear":{"a":9.99e-7,"b":7.7e-8}},"unlockStakeEpoch":99},"protocolConstants":{"k":37,"protocolMagic":1783847074,"vssMaxTTL":1477558317,"vssMinTTL":744040476},"initializer":{"testBalance":{"poors":2448641325904532856,"richmen":14071205313513960321,"totalBalance":10953275486128625216,"richmenShare":4.2098713311249885,"useHDAddresses":true},"fakeAvvmBalance":{"count":17853231730478779264,"oneBalance":15087947214890024355},"avvmBalanceFactor":0.366832547637728,"useHeavyDlg":false,"seed":0}}}
Copy link
Member

@erikd erikd Aug 15, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Curious about what changed here :)

Maybe we could use aeson-pretty to as part of these JSON golden tests to make these a little more human readable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea - I think that would make the tests easier to inspect. Although, when a mismatch does occur, goldenTestBi does a good job of highlighting the diff.

The change here was that many of the keys (newtype wrappers around ByteString) were too short, and the decoders check data invariants, so were complaining. It's hard to tell from that diff, of course 🙂

Should I make the change in this PR or in another one? I've been trying to smash these ones out quickly so I can get back to the meat of CO-354.

Copy link
Member

Choose a reason for hiding this comment

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

If you just want to merge this and get back to CO-354, raise a YT ticket for yourself :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just raised a ticket here.

erikd
erikd approved these changes Aug 16, 2018
View reviewed changes
Copy link
Member

@erikd erikd left a comment

Choose a reason for hiding this comment

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

I'm ok with seeing this merged now if there is a ticket raised for the issues I raised.

Jimbo4350
Jimbo4350 approved these changes Aug 16, 2018
View reviewed changes
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.

LGTM

@mhuesch
[CDEC-506] Fix GenesisConfiguration_GCSpec JSON test
1d9316b 
A number of fields were of the wrong lengths, which wasn't caught
on `encode`ing, but is checked on `decode`ing. The last commit added
a decoding check to `goldenTestJSON`, which caught this error.

This commit fixes up the ByteString fields so the test passes.
@mhuesch mhuesch merged commit fe893f9 into develop Aug 16, 2018
@mhuesch mhuesch deleted the mhuesch/CDEC-506 branch August 16, 2018 14:23
@Jimbo4350 Jimbo4350 mentioned this pull request Aug 23, 2018
Closed
11 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Jimbo4350 Jimbo4350 Jimbo4350 approved these changes

@erikd erikd erikd approved these changes

@intricate intricate Awaiting requested review from intricate

@ruhatch ruhatch Awaiting requested review from ruhatch

Assignees

@mhuesch mhuesch

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mhuesch @erikd @Jimbo4350