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

Fixed LedgerPeers generators #5092

Merged
merged 3 commits into from
Mar 7, 2025
Merged

Fixed LedgerPeers generators #5092

merged 3 commits into from
Mar 7, 2025

Conversation

coot
Copy link
Contributor

@coot coot commented Mar 7, 2025

Description

  • testnet: iosim & iosimpor properties
  • testnet: updated NodeArgs Show instance
  • ledger-peers: fixed ledger peers QuickCheck generators

Fixes: #5091

Checklist

Quality

  • Commit sequence makes sense and have useful messages, see ref.
  • New tests are added and existing tests are updated.
  • Self-reviewed the PR.

Maintenance

  • Linked an issue or added the PR to the current sprint of ouroboros-network project.
  • Added labels.
  • Updated changelog files.
  • The documentation has been properly updated, see ref.

coot added 2 commits March 6, 2025 16:10
When properties are defined as functions, it is easier to paste shrinked
counterexamples.
@coot coot requested a review from a team as a code owner March 7, 2025 07:48
@coot coot added testing testnet Issues / PRs related to the simulation testnet labels Mar 7, 2025
@coot coot self-assigned this Mar 7, 2025
@coot coot force-pushed the coot/ledger-peers-generators branch from df9dbc4 to 46c736f Compare March 7, 2025 07:59
Copy link
Contributor

@bolt12 bolt12 left a comment

Choose a reason for hiding this comment

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

LGTMjust some small comments

Comment on lines 433 to 434
<- listOf1 (listOf1 (sublistOf (makeRelayAccessPoint <$> ledgerPeersRelays)
`suchThat` (not . null)))
Copy link
Contributor

Choose a reason for hiding this comment

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

using suchThat might make the generator slower. Perhaps we can generate directly from NonEmpty ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we need sublistOf1 which is not in QuickCheck

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added 😁, for now it lives in this file; once we'll need it elsewhere we can move it to ouroboros-network-testing.

Copy link
Contributor

@bolt12 bolt12 Mar 7, 2025

Choose a reason for hiding this comment

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

Right, that's because you can't have a sublistOf1 of the empty list. I took a look at your implementation. I found this one to be simpler

sublistOf1 :: NonEmpty a -> Gen [a]
sublistOf1 (NonEmpty xs) = do
    (e:listWithoutE) <- pick xs
    subList <- sublistOf listWithoutE
    pure (e:subList)
  where
  	pick :: NonEmpty a -> Gen (a, [a])
  	pick (NonEmpty as) = do
  	  i <- choose (0, length as - 1)
      case splitAt i as of
		(l, (a:as')) -> pure (a, l ++ as')
		(l, [])      -> pick l

I wrote it here without any LSP help, might need some help compiling 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My implementation preserves the order of elements.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah right we have to preserve that, it shouldn't be hard to tweak mine to fix it without requiring the impartiality from yours, no?

The `naLedgerPeers` requires a script of ledger peers.  We used to
generate just a single ledger peer, instead generate a list of them.

The ledger peer generation had a devision by zero error, which is fixed
in this commit as well.
@coot coot force-pushed the coot/ledger-peers-generators branch from 46c736f to 2e041df Compare March 7, 2025 15:54
@coot coot enabled auto-merge March 7, 2025 15:57
@coot coot added this pull request to the merge queue Mar 7, 2025
Merged via the queue into main with commit b2cf8f4 Mar 7, 2025
13 checks passed
@coot coot deleted the coot/ledger-peers-generators branch March 7, 2025 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testing testnet Issues / PRs related to the simulation testnet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

connection manager test failure
2 participants