Throw spv errors as pact errors with type ContinuationError#1220
Throw spv errors as pact errors with type ContinuationError#1220edmundnoble merged 5 commits intomasterfrom
Conversation
emilypi
left a comment
emilypi
left a comment
There was a problem hiding this comment.
Fastest fork in the west
sirlensalot
left a comment
sirlensalot
left a comment
There was a problem hiding this comment.
Need to document what code coverage options are available for this change and why or why not to require them. Also need to document impact on replay.
|
I've added a test of the error type, re-requesting review. I made some mechanical changes to these tests:
|
tests/PactContinuationSpec.hs
Outdated
| succeedsWith :: HasCallStack => Command Text -> (PactValue -> Expectation) -> | ||
| ReaderT (HM.HashMap RequestKey (CommandResult Hash)) IO () | ||
| succeedsWith cmd r = succeedsWith' cmd ((,[]) <$> r) | ||
| succeedsWith cmd r = succeedsWith' cmd (\(pv,es) -> (es `shouldBe` []) *> r pv) |
There was a problem hiding this comment.
👍 I like these changes to the testing dsl
jwiegley
left a comment
jwiegley
left a comment
There was a problem hiding this comment.
This PR makes such extensive changes to the testing API, that it's hard to see what the changes are relating only to ContinuationError.
|
The changes appear extensive in the diff, but they're explained above as mechanical changes that are easy to audit. As is, we have a few helpers for writing assertions about nested data, but composing them is difficult when the data becomes more deeply nested, and when we need to assert on multiple fields. All I did was make the language for assertions on PactResult slightly more composable. I don't think we have time for me to convince you of the benefits of this approach now if it's not obvious, so I'll revert the larger part of the changes. Maybe at a future Kadena tech talk? |
sirlensalot
left a comment
sirlensalot
left a comment
There was a problem hiding this comment.
Tests look good, thanks for the cleanup in aisle crufty-seven
|
@jwiegley while the changes look like a lot, it was really a teetering tower of cruft that @edmundnoble has tamed, as the author of said cruft I can confirm it's better now. Finally it was probably the only way to get coverage for the error type without making things even more cruftastic. |
|
Pre-fork test looks good, LGTM to merge on CI |
5 participants
No description provided.