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

Add golden tests to ProtocolParameters serialization #457

Merged
merged 30 commits into from
Apr 16, 2024
Merged

Conversation

palas
Copy link
Contributor

@palas palas commented Feb 18, 2024

This PR:

  • Adds a golden test for ProtocolParameters
  • Adds property based tests for comparing ProtocolParameters and PParams for the different eras
  • Fixes the implementation of toAlonzoPParams

In order to make the tests work, this PR is built upon a branch of an old commit of cardano-ledger.

The rebased version of this branch is proposed in this PR in cardano-ledger.

Changelog

- description: |
    Added a golden test for ProtocolParameters, added three property-based tests for comparing ProtocolParameters and PParams for the different eras, and fixed the implementation of `toAlonzoPParams` function.
  type:
  - improvement    # QoL changes e.g. refactoring

Context

This is part of an effort to unify the JSON instances of ProtocolParameters and PParams to eventually migrate and use PParams directly in cardano-api instead of ProtocolParameters

How to trust this PR

Check that the properties and golden tests are the right ones. And check that the changes I did to the conversion functions make sense. Possible check together with this PR in cardano-ledger.

Checklist

  • Commit sequence broadly makes sense and commits have useful messages
  • New tests are added if needed and existing tests are updated. See Running tests for more details
  • Self-reviewed the diff

@palas palas self-assigned this Feb 18, 2024
@palas palas force-pushed the unify-json-instances branch 4 times, most recently from 18c228d to 1fecdea Compare February 26, 2024 05:38
@palas palas force-pushed the unify-json-instances branch 2 times, most recently from f9359fd to 0a37fa1 Compare February 26, 2024 20:55
@palas palas changed the title DO NOT MERGE (just for discussion) - Add golden tests to ProtocolParameters serialization Add golden tests to ProtocolParameters serialization Feb 27, 2024
@palas palas force-pushed the unify-json-instances branch from 1c8a727 to 902285b Compare February 28, 2024 22:51
@palas palas marked this pull request as ready for review February 29, 2024 04:55
@palas palas force-pushed the unify-json-instances branch 4 times, most recently from f9d86e1 to 2f0e2e4 Compare March 1, 2024 01:05
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.

Looking good 👍

newhoggy pushed a commit that referenced this pull request Mar 11, 2024
…ed-to-non-extended-golden-tests

Add one missing extended to non extended golden tests
@palas palas force-pushed the unify-json-instances branch 3 times, most recently from 7389451 to 6cceb23 Compare March 15, 2024 20:14
@palas palas requested a review from Jimbo4350 March 15, 2024 20:53
@palas palas force-pushed the unify-json-instances branch from 6abcb58 to 4ac07fa Compare April 15, 2024 16:20
@palas palas added this pull request to the merge queue Apr 15, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Apr 15, 2024
@palas palas added this pull request to the merge queue Apr 16, 2024
Merged via the queue into main with commit 3204650 Apr 16, 2024
21 checks passed
@palas palas deleted the unify-json-instances branch April 16, 2024 00:24
@smelc
Copy link
Contributor

smelc commented May 30, 2024

Contributes to fixing #384

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Adding or reworking tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants