Skip to content

Add goldenEvalCekCatchBudget and change existing cases to use it #7093

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

Merged
merged 6 commits into from
May 16, 2025

Conversation

SeungheonOh
Copy link
Collaborator

@SeungheonOh SeungheonOh commented May 13, 2025

goldenEvalCekCatchBudget combines goldenBudget and goldenEvalCekCatch. Unlike calling two separately, this will only run CEK once.

Previous cases that used goldenBudget will use goldenEvalCekCatchBudget since it's more sensible to test their evaluation result as well.

Also, changed some cases to use goldenBundle'. Looks like these were missed out from #7057.

Closes #6846

`goldenEvalCekCatchBudget` combines `goldenBudget` and
`goldenEvalCekCatch`. Unlike calling two separately, this will only
run CEK once.
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.

Less noise is always a good thing.

`goldenEvalCekCatchBudget` should be used instead
Now argument is just `toUPlc a => a` not `[a]`
@ana-pantilie ana-pantilie added the No Changelog Required Add this to skip the Changelog Check label May 14, 2025
@ana-pantilie
Copy link
Contributor

@SeungheonOh I added the "no changelog required" label. It's fine to add it to make CI happy if your changes aren't user facing (like in this case).

@SeungheonOh
Copy link
Collaborator Author

I don't have permission to add labels nor merge anything(even with CI passing and approved review!) yet. For now, please feel free to merge this whenever CI's happy.

Copy link
Member

@zliu41 zliu41 left a comment

Choose a reason for hiding this comment

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

Good work 👍

nestedGoldenVsDocM name ".eval" $ do
either (pretty . show) prettyPlcClassicSimple
<$> runExceptT (fst <$> evalRes)
nestedGoldenVsDocM name ".budget" $ ppCatch $ do
Copy link
Member

Choose a reason for hiding this comment

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

I think it's better to create one file containing both budget and result, instead of two?

Copy link
Contributor

@effectfully effectfully May 14, 2025

Choose a reason for hiding this comment

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

I think so too. Budget should go first in case the result is large (can happen for early Plutus versions of if error message is long).

Copy link
Collaborator Author

@SeungheonOh SeungheonOh May 15, 2025

Choose a reason for hiding this comment

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

My reasoning here is based on this comment by @Unisay : #6846 (comment)

Having two files will give better feedback: one can easily check if budgeting is broken or evaluator is broken or both are broken from file changes without going over each files

Copy link
Member

Choose a reason for hiding this comment

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

I disagree - you don't need to go over the entire file if the budget changed and the result didn't - the diff would clearly show that, especially if the budget appears before the result like @effectfully said.

Having both in a single file makes it easier to review the change (which benefits both the author and the reviewers): when the budget changes, I always want to verify whether the result changed too. Putting them in separate files means I'd need to look for the corresponding .eval file, which may not be adjacent to the .budget file, or included in the PR. With a single file, I can scan through changes much more efficiently.

Copy link
Member

@zliu41 zliu41 left a comment

Choose a reason for hiding this comment

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

There are some golden files that need to be removed, e.g. checkScriptContext2-20.budget.golden. You can delete all golden files and then re-run the tests.

Copy link
Member

@zliu41 zliu41 left a comment

Choose a reason for hiding this comment

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

Try

cabal test plutus-tx-plugin --test-options=--accept
cabal test plutus-benchmark --test-options=--accept
cabal test plutus-ledger-api:plutus-ledger-api-plugin-test --test-options=--accept
cabal test cardano-constitution --test-options=--accept

This should fix all the golden files.

@zliu41 zliu41 merged commit ffdcada into IntersectMBO:master May 16, 2025
23 of 25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
No Changelog Required Add this to skip the Changelog Check
Projects
None yet
Development

Successfully merging this pull request may close these issues.

goldenBudget should show evaluation result
5 participants