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

[CDEC-500] Add golden tests for Address #3405

Merged
merged 2 commits into from
Aug 15, 2018
Merged

[CDEC-500] Add golden tests for Address #3405

merged 2 commits into from
Aug 15, 2018

Conversation

mhuesch
Copy link
Contributor

@mhuesch mhuesch commented Aug 15, 2018

Description

[CDEC-500] Add golden tests for Address

Since we are modifying this for CO-354, and it's quite important to
preserve backwards compatibility of Addresses, I've added 5 golden
tests to ensure the various configurations of Addresses will have
proper test coverage.

Linked issue

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

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.
    ^ we're adding test coverage, which is not very noteworthy, and can be mentioned in a CHANGELOG entry for the parent YT ticket.

Testing checklist

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

The `Bi` & `Json` modules had a number of datatypes which were shared
and could be factored out. This commit pulls them out into the
`ExampleHelpers` module.
@mhuesch mhuesch changed the title Mhuesch/cdec 500 2 [CDEC-500] Add golden tests for Address Aug 15, 2018
@mhuesch mhuesch force-pushed the mhuesch/CDEC-500-2 branch from a85c456 to e5faa88 Compare August 15, 2018 04:26
Since we are modifying this for CO-354, and it's quite important to
preserve backwards compatibility of Addresses, I've added 5 golden
tests to ensure the various configurations of Addresses will have
proper test coverage. 4 example Address datatypes are added to the
1 existing datatype, and all 5 are used for golden tests in both
the `Json` and `Bi` modules.
@mhuesch
Copy link
Contributor Author

mhuesch commented Aug 15, 2018

This must be merged after #3403, which contains the prerequisite refactoring commit that's included in this branch.

My hope is that once #3403 is merged, this PR will apply the second commit cleanly onto develop.

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.

Looks good, just a question WRT JSON encoding.

@@ -0,0 +1 @@
"2RhQhCGqYPDpFgTsnBTbnvPvCwpqAkjwLqQkWpkqXbLRmNxd4xNd262nGsr8JiynyKRUeMLSJ9Ntho9i76uvBTrVXdJJG5yiNLb8frmUe5qX7E"
Copy link
Contributor

@Jimbo4350 Jimbo4350 Aug 15, 2018

Choose a reason for hiding this comment

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

Was this JSON encoded? Same question for the other golden address files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah good question - that should've raised eyebrows for me but I was too focused on finishing the job to notice.

It seems, though, like that's just the JSON format of Address for us: toJSON calls toObjectKey which uses addressF to call build.

Let me know what you think, since you've worked in the Json parts of the codebase more.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep this is the right encoding, just double checked in GHCi. Definitely looks different to the usual key/value map style encoding.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool, thanks for confirming.

@mhuesch mhuesch merged commit 56ba989 into develop Aug 15, 2018
@mhuesch mhuesch deleted the mhuesch/CDEC-500-2 branch August 15, 2018 12:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants