-
Notifications
You must be signed in to change notification settings - Fork 16
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
Make ErrorsSpec
use generated types and constructors names
#9
Conversation
326f5af
to
e2b8dcc
Compare
$ displayError $ ScriptDataRangeError (Aeson.String "<JSON>") (ScriptDataConstructorOutOfRange 1) | ||
|
||
it "ScriptDataJsonSchemaError" $ H.requireTest $ do | ||
flip H.diffVsGoldenFile "test/golden/errors/ScriptDataJsonSchemaError/ScriptDataJsonSchemaError.txt" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a typo with file name which went unnoticed. The golden file name didn't match the constructor name. Hopefully it will be harder to make new ones.
displayError err `H.diffVsGoldenFile` ("test/golden/errors" </> typeName </> constructorName <> ".txt") | ||
testErrorMessage_ moduleName typeName constructorName err | ||
|
||
testErrorMessage_ :: (HasCallStack, Error a) => String -> String -> String -> a -> Spec |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An escape hatch when adding of Data
instance gets impossible (like when we embed TypeRep
in our error data types) or requires significant multi-package changes and outweights the benefits here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add your comment to the code 👍
, Bech32WrongPrefix text text | ||
] | ||
|
||
testAllErrorMessages |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be useful to use type applications here and elsewhere so the type name is in the test code. For example:
testAllErrorMessages @InputDecodeError
e2b8dcc
to
751c8b0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
displayError err `H.diffVsGoldenFile` ("test/golden/errors" </> typeName </> constructorName <> ".txt") | ||
testErrorMessage_ moduleName typeName constructorName err | ||
|
||
testErrorMessage_ :: (HasCallStack, Error a) => String -> String -> String -> a -> Spec |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add your comment to the code 👍
|
||
testErrorMessage :: (HasCallStack, MonadIO m, MonadTest m, Data p , Error p) => p -> m () | ||
-- , ("TransactionValidityIntervalError", TransactionValidityIntervalError $ | ||
-- Qry.PastHorizon |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left over comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a leftover from original change, that we want to eventually test this case as well, but we can't because we don't have access to data constructors of PastHorizon
or its fields' types' constructors and we need to expose a dummy value for such purposes.
, TxMetadataBytesTooLong 0 | ||
, TxMetadataNumberOutOfRange 0 | ||
] | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally we want to check that we have covered all of the constructors. I suggest the following (can be a follow up PR):
diff --git a/cardano-api/golden/Test/Golden/ErrorsSpec.hs b/cardano-api/golden/Test/Golden/ErrorsSpec.hs
index 036fe5db0..d6c1d4149 100644
--- a/cardano-api/golden/Test/Golden/ErrorsSpec.hs
+++ b/cardano-api/golden/Test/Golden/ErrorsSpec.hs
@@ -237,11 +237,16 @@ spec = describe "Test.Golden.Errors" $ do
testAllErrorMessages @TxMetadataRangeError
[ TxMetadataTextTooLong 0
, TxMetadataBytesTooLong 0
- , TxMetadataNumberOutOfRange 0
+ -- , TxMetadataNumberOutOfRange 0
]
+countDataConstructors :: Data a => a -> Int
+countDataConstructors = length . dataTypeConstrs . dataTypeOf
+
testAllErrorMessages :: forall a. (HasCallStack, Data a, Error a) => [a] -> Spec
testAllErrorMessages errs = withFrozenCallStack $ do
+ it "All data constructors covered" $ do
+ length errs `shouldBe`countDataConstructors (head errs)
let typeName = show $ typeRep (Proxy @a)
describe typeName $
mapM_ testErrorMessage errs
When you run the golden test suite:
...
✓ <interactive> passed 1 test.
TxMetadataRangeError [✔]
All data constructors covered [✘]
TxMetadataRangeError
✓ <interactive> passed 1 test.
TxMetadataTextTooLong [✔]
✓ <interactive> passed 1 test.
TxMetadataBytesTooLong [✔]
Failures:
1) Test.Golden.Errors.Test.Golden.Errors All data constructors covered
expected: 3
but got: 2
To rerun use: --match "/Test.Golden.Errors/Test.Golden.Errors/All data constructors covered/"
You should probably pattern match on the empty list and fail in that case so we can "safely" use head
or something similar.
751c8b0
to
9ad6061
Compare
9ad6061
to
b01534b
Compare
Description
All important changes are in
ErrorSpec
modules. Other modified haskell source files containData
instances required for using derived types', modules' and constructors' names.Changelog
Checklist
See Runnings tests for more details
CHANGELOG.md
for affected package.cabal
files are updatedhlint
. See.github/workflows/check-hlint.yml
to get thehlint
versionstylish-haskell
. See.github/workflows/stylish-haskell.yml
to get thestylish-haskell
versionghc-8.10.7
andghc-9.2.7
Note on CI
If your PR is from a fork, the necessary CI jobs won't trigger automatically for security reasons.
You will need to get someone with write privileges. Please contact IOG node developers to do this
for you.