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

Ledger types testing #6618

Closed
wants to merge 2 commits into from

Conversation

aleeusgr
Copy link
Contributor

@aleeusgr aleeusgr commented Oct 31, 2024

This is an experiment, it consists of two parts:

  • upstreaming Arbitrary instances from plutarch to plutus-ledger-api
  • asserting the value of proposition "all types exported by plutus ledger api provide arbitraries".

Pre-submit checklist:

  • Branch
    • Tests are provided (if possible)
    • Commit sequence broadly makes sense
    • Key commits have useful messages
    • Changelog fragments have been written (if appropriate)
    • Relevant tickets are mentioned in commit messages
    • Formatting, PNG optimization, etc. are updated
  • PR
    • (For external contributions) Corresponding issue exists and is linked in the description QuickCheck Arbitrary instances for Ledger types #6210
    • Targeting master unless this is a cherry-pick backport
    • Self-reviewed the diff
    • Useful pull request description
    • Reviewer requested

@@ -136,6 +137,8 @@ tests = testGroup "plutus-ledger-api"
, Spec.CostModelParams.tests
, Spec.ContextDecoding.tests
, Value.test_Value
, Value.test_FaceValue
, OutputDatum.testOutputDatum
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I get errors:

No instance for ‘Arbitrary DatumHash’                                                                            
        arising from a use of ‘arbitrary’
...
No instance for ‘Arbitrary Datum’                                                                            
        arising from a use of ‘arbitrary’

Copy link
Contributor

Choose a reason for hiding this comment

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

Not here though, in OutputDatum.

Copy link
Contributor

@effectfully effectfully left a comment

Choose a reason for hiding this comment

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

This one isn't gonna cut it, sorry. It doesn't test anything useful and for it to build you basically need to draw the rest of the owl.

#6624 seems to have the whole owl, so I'm gonna review that PR and then we'll decide what to do with this one.

Meanwhile, maybe I can interest you in resolving some simple issues like this one?

@@ -136,6 +137,8 @@ tests = testGroup "plutus-ledger-api"
, Spec.CostModelParams.tests
, Spec.ContextDecoding.tests
, Value.test_Value
, Value.test_FaceValue
, OutputDatum.testOutputDatum
Copy link
Contributor

Choose a reason for hiding this comment

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

Not here though, in OutputDatum.

test_roundTripFaceValue :: TestTree
test_roundTripFaceValue = testProperty "unFaceValue followed by FaceValue returns the original" $
\(faceValue :: FaceValue) ->
unFaceValue (FaceValue $ unFaceValue faceValue) === unFaceValue faceValue
Copy link
Contributor

Choose a reason for hiding this comment

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

That's a completely useless test unfortunately, this property is guaranteed by GHC.

@@ -69,6 +69,9 @@ instance Arbitrary FaceValue where
, (1, FaceValue . fromIntegral <$> arbitrary @Int)
]

instance Show FaceValue where
show (FaceValue value) = show value
Copy link
Contributor

Choose a reason for hiding this comment

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

You could just derive it, but OK.

oneof
[ pure NoOutputDatum
, OutputDatumHash <$> arbitrary
, OutputDatum <$> arbitrary
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, that relies on a whole lot other Arbitrary instances existing.

@aleeusgr
Copy link
Contributor Author

aleeusgr commented Nov 9, 2024

Thank you!

I hoped I could build up from here to the whole picture but since it was completed by someone else it makes no sense to spend time on it.

I'll look into another issue, thank you - I was looking for something else to do.

@aleeusgr aleeusgr closed this Nov 13, 2024
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.

2 participants