Skip to content

Refactor and modernise DRep golden tests for thread-safety#1209

Closed
newhoggy wants to merge 4 commits into
masterfrom
newhoggy/thread-safe-golden-tests-with-hedgehog-extras
Closed

Refactor and modernise DRep golden tests for thread-safety#1209
newhoggy wants to merge 4 commits into
masterfrom
newhoggy/thread-safe-golden-tests-with-hedgehog-extras

Conversation

@newhoggy
Copy link
Copy Markdown
Contributor

@newhoggy newhoggy commented Jun 1, 2025 •
edited
Loading

Changelog

- description: |
    Refactor and modernise `DRep` golden tests for thread-safety
# 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...

Context

This PR improves the structure and exception safety of golden tests related to DRep governance features by:

  • Replacing Property-based Hedgehog tests with a new UnitIO style enabled by Hedgehog.Extras.Test.New.Monad from hedgehog-extras, which simplifies composition and improves readability.
  • Extracting serveFilesWhile to two new modules:
    • Test.Cardano.CLI.Network (used in traditional test code)
    • Test.Cardano.CLI.New.Network (uses MonadMask, preferred in the new-style tests)

Dependency Update

Updates the hedgehog-extras dependency, for the new support for the UnitIO abstraction and new test monads.

Benefits

  • Clearer test structure and improved type safety via UnitIO.
  • Better exception handling guarantees using MonadMask.
  • Isolated network test utilities support both legacy and modern test styles.

How to trust this PR

Highlight important bits of the PR that will make the review faster. If there are commands the reviewer can run to observe the new behavior, describe them.

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 reviewed Jun 2, 2025
View reviewed changes
Comment thread cardano-cli/test/cardano-cli-golden/Test/Golden/Governance/DRep.hs
-- @cabal test cardano-cli-golden --test-options '-p "/golden governance drep id hash/"'@
hprop_golden_governance_drep_id_hash :: Property
hprop_golden_governance_drep_id_hash =
watchdogProp . propertyOnce $ do
Copy link
Copy Markdown
Contributor

@carbolymer carbolymer Jun 2, 2025

Choose a reason for hiding this comment

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

I think we should keep watchdogs everywhere.

Copy link
Copy Markdown
Contributor Author

@newhoggy newhoggy Jun 2, 2025

Choose a reason for hiding this comment

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

We will need to write a new watchdog for the new test api. I wanted to see it hang in one of these property test first before adding it.

@newhoggy newhoggy force-pushed the newhoggy/thread-safe-golden-tests-with-hedgehog-extras branch from 8baf41b to 8d3138d Compare June 2, 2025 14:34
@Jimbo4350 Jimbo4350 self-requested a review June 2, 2025 18:36
Jimbo4350
Jimbo4350 requested changes Jun 2, 2025
View reviewed changes
Copy link
Copy Markdown
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.

I'm not convinced this refactor is necessary. Let's explore other options.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jul 18, 2025

This PR is stale because it has been open 45 days with no activity.

@github-actions github-actions Bot added the Stale label Jul 18, 2025
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Sep 17, 2025

This issue was closed because it has been stalled for 60 days with no activity.

@github-actions github-actions Bot closed this Sep 17, 2025
@Jimbo4350 Jimbo4350 mentioned this pull request May 14, 2026
Closed
3 tasks
Jimbo4350 added a commit that referenced this pull request May 14, 2026
@Jimbo4350
Migrate to experimental TxOut in createCompatibleTx
496fd98 
Adapt to cardano-api PR #1209, which removes the legacy 'TxOut CtxTx era'
from the Compatible and Experimental APIs:

  * 'createCompatibleTx' now takes '[Exp.TxOut (ShelleyLedgerEra era)]'
    plus a new 'Map L.DataHash (L.Data ...)' argument carrying any
    supplemental datum bodies. The legacy 'TxOut CtxTx era' bundled
    supplemental datums inside outputs; 'Exp.TxOut' only carries the
    datum hash, so callers thread the full datum bodies in explicitly.
  * The bridge helpers 'fromLegacyTxOut', 'legacyDatumToDatum',
    'supplementalDatumFromLegacy', 'toLedgerDatum', and the
    'DatumDecodingError' type are deleted from 'Cardano.Api.Experimental.Tx'.

Migration:

  * 'mkTxOut' and 'toTxOutInAnyEra' now return
    '(Exp.TxOut (ShelleyLedgerEra era), Map DataHash (L.Data ...))'
    directly, building the legacy 'TxOut CtxTx era' internally only as
    a stepping stone for 'toShelleyTxOutAny'.
  * 'createCompatibleTx' call site in 'Compatible/Transaction/Run.hs'
    folds the per-output supplemental datums via 'Map.unions' and
    passes them as the new argument.
  * 'toTxOutInEra' and 'toTxOutInShelleyBasedEra' in
    'EraBased/Transaction/Run.hs' delegate directly to 'mkTxOut' (the
    deleted 'fromLegacyTxOut' is gone).
  * 'TxCmdDatumDecodingError' is removed since the underlying
    'Exp.DatumDecodingError' is gone and the new conversion is total.

Temporary 'cabal.project' additions (to be removed once #1209 is
merged and the next cardano-api is published to CHaP):

  * 'source-repository-package' pointing at the PR branch so CI can
    build against the unpublished API.
  * Per-package '-Wwarn=deprecations -Wwarn=unused-imports' for
    cardano-cli. The PR branch is several commits ahead of
    cardano-api-11.1.0.0 and surfaces unrelated TxBody/TxBodyContent
    deprecations (cardano-api PR #1200) plus a redundant
    'Cardano.Ledger.Core' import in 'Cardano.CLI.Read' that became
    visible after upstream re-exports widened. Both are separate
    cleanups out of scope for this PR.
This was referenced May 18, 2026
Open
Closed
Jimbo4350 added a commit that referenced this pull request May 19, 2026
@Jimbo4350
Migrate to experimental TxOut in createCompatibleTx
dda41f7 
Adapt to cardano-api PR #1209, which removes the legacy 'TxOut CtxTx era'
from the Compatible and Experimental APIs:

  * 'createCompatibleTx' now takes '[Exp.TxOut (ShelleyLedgerEra era)]'
    plus a new 'Map L.DataHash (L.Data ...)' argument carrying any
    supplemental datum bodies. The legacy 'TxOut CtxTx era' bundled
    supplemental datums inside outputs; 'Exp.TxOut' only carries the
    datum hash, so callers thread the full datum bodies in explicitly.
  * The bridge helpers 'fromLegacyTxOut', 'legacyDatumToDatum',
    'supplementalDatumFromLegacy', 'toLedgerDatum', and the
    'DatumDecodingError' type are deleted from 'Cardano.Api.Experimental.Tx'.

Migration:

  * 'mkTxOut' and 'toTxOutInAnyEra' now return
    '(Exp.TxOut (ShelleyLedgerEra era), Map DataHash (L.Data ...))'
    directly, building the legacy 'TxOut CtxTx era' internally only as
    a stepping stone for 'toShelleyTxOutAny'.
  * 'createCompatibleTx' call site in 'Compatible/Transaction/Run.hs'
    folds the per-output supplemental datums via 'Map.unions' and
    passes them as the new argument.
  * 'toTxOutInEra' and 'toTxOutInShelleyBasedEra' in
    'EraBased/Transaction/Run.hs' delegate directly to 'mkTxOut' (the
    deleted 'fromLegacyTxOut' is gone).
  * 'TxCmdDatumDecodingError' is removed since the underlying
    'Exp.DatumDecodingError' is gone and the new conversion is total.

Temporary 'cabal.project' additions (to be removed once #1209 is
merged and the next cardano-api is published to CHaP):

  * 'source-repository-package' pointing at the PR branch so CI can
    build against the unpublished API.
  * Per-package '-Wwarn=deprecations -Wwarn=unused-imports' for
    cardano-cli. The PR branch is several commits ahead of
    cardano-api-11.1.0.0 and surfaces unrelated TxBody/TxBodyContent
    deprecations (cardano-api PR #1200) plus a redundant
    'Cardano.Ledger.Core' import in 'Cardano.CLI.Read' that became
    visible after upstream re-exports widened. Both are separate
    cleanups out of scope for this PR.
Jimbo4350 added a commit that referenced this pull request May 19, 2026
@Jimbo4350
Bump cardano-api to 11.2.0.0
29adccd 
cardano-api 11.2.0.0 is published to CHaP. Bump the
cardano-haskell-packages index-state past the publish
(2026-05-18T18:23:40Z), bump the cardano-cli cabal dep from ^>=11.1
to ^>=11.2, and bump the CHaP flake input accordingly.

11.2.0.0 introduces breaking changes that subsequent commits in this
PR migrate cardano-cli to: legacy TxOut removed (cardano-api #1209),
legacy Certificate type removed (#1210), and TxBody/TxBodyContent
deprecated (#1200).
Jimbo4350 added a commit that referenced this pull request May 19, 2026
@Jimbo4350
Migrate to experimental TxOut in createCompatibleTx
65ef5d2 
Adapt to cardano-api PR #1209, which removes the legacy 'TxOut CtxTx era'
from the Compatible and Experimental APIs:

  * 'createCompatibleTx' now takes '[Exp.TxOut (ShelleyLedgerEra era)]'
    plus a new 'Map L.DataHash (L.Data ...)' argument carrying any
    supplemental datum bodies. The legacy 'TxOut CtxTx era' bundled
    supplemental datums inside outputs; 'Exp.TxOut' only carries the
    datum hash, so callers thread the full datum bodies in explicitly.
  * The bridge helpers 'fromLegacyTxOut', 'legacyDatumToDatum',
    'supplementalDatumFromLegacy', 'toLedgerDatum', and the
    'DatumDecodingError' type are deleted from 'Cardano.Api.Experimental.Tx'.

Migration:

  * 'mkTxOut' and 'toTxOutInAnyEra' now return
    '(Exp.TxOut (ShelleyLedgerEra era), Map DataHash (L.Data ...))'
    directly, building the legacy 'TxOut CtxTx era' internally only as
    a stepping stone for 'toShelleyTxOutAny'.
  * 'createCompatibleTx' call site in 'Compatible/Transaction/Run.hs'
    folds the per-output supplemental datums via 'Map.unions' and
    passes them as the new argument.
  * 'toTxOutInEra' and 'toTxOutInShelleyBasedEra' in
    'EraBased/Transaction/Run.hs' delegate directly to 'mkTxOut' (the
    deleted 'fromLegacyTxOut' is gone).
  * 'TxCmdDatumDecodingError' is removed since the underlying
    'Exp.DatumDecodingError' is gone and the new conversion is total.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@carbolymer carbolymer carbolymer left review comments

@Jimbo4350 Jimbo4350 Jimbo4350 requested changes

@palas palas Awaiting requested review from palas palas will be requested when the pull request is marked ready for review palas is a code owner

@kevinhammond kevinhammond Awaiting requested review from kevinhammond kevinhammond will be requested when the pull request is marked ready for review kevinhammond is a code owner

@disassembler disassembler Awaiting requested review from disassembler disassembler will be requested when the pull request is marked ready for review disassembler is a code owner

Copilot code review Copilot Awaiting requested review from Copilot Copilot will automatically review once the pull request is marked ready for review

Assignees

No one assigned

Labels

Stale

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@newhoggy @carbolymer @Jimbo4350