Skip to content

Conversation

@carbolymer
Copy link
Contributor

Changelog

- description: |
    Add canonical CBOR property test
# uncomment types applicable to the change:
  type:
  # - feature        # introduces a new feature
  # - breaking       # the API has changed in a breaking way
  # - compatible     # the API has changed but is non-breaking
  # - optimisation   # measurable performance improvements
  # - refactoring    # QoL changes
  # - bugfix         # fixes a defect
   - test           # fixes/modifies tests
  # - maintenance    # not directly related to the code
  # - release        # related to a new release preparation
  # - documentation  # change in code docs, haddocks...
# uncomment at least one main project this PR is associated with
  projects:
   - cardano-api
  # - cardano-api-gen
  # - cardano-rpc
  # - cardano-wasm

Context

This PR reverts partially changes made to the test in: https://github.com/IntersectMBO/cardano-api/pull/1047/files

It also adds a property test, which recursively validates canonicalisation of CBOR accordingly to CIP-21.

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

@carbolymer carbolymer force-pushed the mgalazyn/test/add-canonical-cbor-property-test branch from 234daa4 to bed74a7 Compare December 29, 2025 19:57
@carbolymer carbolymer self-assigned this Dec 29, 2025
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.

You are weakening the unit_canonicalise_cbor property test. Why not leave it as it is and just add your prop_canonicalise_cbor property?

@carbolymer
Copy link
Contributor Author

@Jimbo4350 correct. That's because the test felt too complicated to understand and debug. Now that part of the code is covered by prop_canonicalise_cbor test, so we are not losing test coverage here.

@Jimbo4350
Copy link
Contributor

Jimbo4350 commented Jan 15, 2026

@Jimbo4350 correct. That's because the test felt too complicated to understand and debug. Now that part of the code is covered by prop_canonicalise_cbor test, so we are not losing test coverage here.

We are losing ease of debuggability. Removing inputMapInIndefiniteList and inputMapInDefiniteList from unit_canonicalise_cbor makes it more difficult to determine if the canonicalization failed due to a CBOR round trip or the canonicalization itself (for those terms). assertWith eventually calls assert which will give us less information vs a direct comparison against the hardcoded TMap in unit_canonicalise_cbor.

Your new property will fail and only render the value that failed:

assertWithM :: (H.MonadTest m, Show p, HasCallStack) => p -> (p -> m Bool) -> m ()
assertWithM v f = GHC.withFrozenCallStack $ do
  result <- f v
  if result
     then H.success
     else do
       noteShow_ v
       H.assert result

I agree it's not easy to read but it can be cleaned up.

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