tests: Move validation tests to the haskell test suite#119
Conversation
5ef3d08 to
8af96fb
Compare
237352c to
8247b96
Compare
acl-cqc
left a comment
There was a problem hiding this comment.
Ok I love what you have achieved here but I'm not quite sure I agree with the means to the end ;-). Is there not a simpler way to achieve this without the bits you have marked BAD, near-repetition of HugrTest, etc., roughly as follows:
Define a new ValidateTest - like your HugrTest, but not containing a TestTree; instead containing an IO ByteString, a FilePath, and a TestName. This is instance IsTest ValidateTest so that (just like your instance IsTests) we can get at the options (you've done this neatly, let's keep that). This implementation "run"s a newly-constructed Test with that TestName that...
- invokes the
IO ByteStringto get a Hugr as bytes - Writes that to the FilePath
- Invokes the validator on that FilePath
If you really want, it could "run" a sequentialTest that makes the Hugr and writes it as first step ("compile"), then sequences that with a "validate" step, although I note sequentialTest wasn't really doing anything for me...
if invoking the validator works then we don't care about the options, but if validation fails then we can check the options and only fail if the options say validation required (which is default).
This does avoid checking the validator is installed; if it's not, and you run the testsuite without disabling validation, you'll still get lots of failures (as now right?), the difference is it'll try (and fail) to run the validator 100 times rather than failing to determine its version only once. So less efficient, but in that scenario you should be Ctrl-C-ing out of that and rerunning with --ignore-validation in any case. (IOW: I think checkValidatorInPath adds more complexity than it's worth.)
I can have a go at this if you like....
| import Data.Tagged (Tagged(..)) | ||
| import Test.Tasty.Options (IsOption(..), flagCLParser) | ||
|
|
||
| data IgnoreValidation = IgnoreValidation Bool |
There was a problem hiding this comment.
This seems fairly clear (True = ignore validation, false = validate) but maybe an enum would be even clearer as that is a bit of a double negative
| data IgnoreValidation = IgnoreValidation Bool | ||
| instance IsOption IgnoreValidation where | ||
| defaultValue = IgnoreValidation False | ||
| parseValue s = if s == "ignore-validation" then Just defaultValue else Nothing |
There was a problem hiding this comment.
just repeating that with some inlining, that's:
if s == "ignore-validation"
then Just (IgnoreValidation False)
else Nothing
never mind the "else Nothing", but...are you sure??
| checkValidatorInPath :: IO Bool | ||
| checkValidatorInPath = do | ||
| (exitCode, output, _) <- readCreateProcessWithExitCode (shell "hugr_validator --version") "" | ||
| if exitCode == ExitSuccess |
There was a problem hiding this comment.
| if exitCode == ExitSuccess | |
| pure (exitCode == ExitSuccess and "hugr_validator 0." `isPrefixOf` output) |
|
|
||
|
|
||
| interpreterTestsForExample :: Bool -> FilePath -> T.Text -> [TestTree] | ||
| interpreterTestsForExample interpreterInPath path start = |
There was a problem hiding this comment.
this "interpreterInPath" is really, "validatorAvailable" right...
It might change your opinion to know that the animus for this change was that I want an easier way to transition from json validation to hugr model validation. I intend for the next version of hugr-validator to consume the model, and the test suite to check that we have >= 0.5.0 and complain if the validator is the wrong version. In principle this could apply to any breaking change to the output format/validator |
Here's how tests appear when the validator is not installed:
they are then treated as failures in the test suite summary, unless the option
--ignore-validationis passed. Then, they look the same but aren't marked as failures (and "hugr_validator not installed" is not in red)