Add test for validateFlatTerm and round trip test for FlatTerm conversions #196

DeepakKapiswe · 2019-03-22T16:53:16Z

Tests for validateFlatTerm function
Travis: GHC versions update
Add round trip property tests for FlatTerm to / from conversion

cborg/tests/Tests/FlatTerm.hs

dcoutts

This is looking good. The first main patch (ignoring the CI one) is good and just needs a few adjustments. The second patch will need re-thinking, at least the property for invalid flat terms. It'd be ok to split this into two if you like so we can commit the first patch sooner.

dcoutts · 2019-03-29T11:01:05Z

cborg/tests/Tests/Properties.hs

+    imp  = fromRef . canonicaliseRef $ x
+    eq   = eqImp t
+    enc  = encodeImp t imp 
+    fn e = fromFlatTerm  (decodeImp (Proxy :: Proxy t)) $ toFlatTerm e


This can be decodeImp t rather than decodeImp (Proxy :: Proxy t), since we already defined the value t as Proxy :: Proxy t (where the latter t is the type).

dcoutts · 2019-03-29T22:18:30Z

cborg/tests/Tests/Properties.hs

+prop_toFromFlatTerm :: forall t. Token t => Proxy t -> t -> Bool
+prop_toFromFlatTerm _ x =    
+
+    liftA2 eq (fn enc) (Right imp) == Right True


This could be expressed a bit more elegantly if you followed the same trick as deserialiseImp which is basically a wrapper that throws an exception of the result is a decode failure.

Here, you'd make a local wrapper around fromFlatTerm that just throws an exception in the Left case. Something like

where deserialiseFromFlatTerm dec = case fromFlatTerm dec flatterm of Left _ -> error "fromFlatTerm: decode failure" Right x -> x

Then your overall property can match the diagram directly (and like in your comment).

(deserialiseFromFlatTerm (decodeImp t) . toFlatTerm . encodeImp t) imp `eq` imp

The idea with these commuting diagrams is that the property can be read off directly by composing all the arrows. So this reads most nicely when the property is exactly that.

Yes, correct Implemented accordingly.

dcoutts · 2019-03-29T22:24:25Z

cborg/tests/Tests/FlatTerm.hs

+
+
+-- Convert Term to List of Valid TermTokens
+termToValidTermTokens :: Term -> [TermToken]


This could be replaced by toFlatTerm . encodeTerm couldn't it?

dcoutts · 2019-03-29T22:29:01Z

cborg/tests/Tests/FlatTerm.hs

+prop_validateValidFlat (ValidFlat ts) = property $ validFlatTerm ts
+
+prop_validateInValidFlat :: InValidFlat -> Property
+prop_validateInValidFlat (InValidFlat ts) = property $ validFlatTerm ts == False


This is a very add-hoc property. The generator for InValidFlat is just using various but fixed ways of making an invalid flat term.

I'm not happy with adding this property. It looks like it's testing something that it's really not testing well.

An alternative approach would be to naively generate arbitrary flat terms and then check that the reference decoder (ie from Tests.Reference.Implementation) agrees with validFlatTerm. By agree, I mean they both agree it's valid, or both agree it's invalid. Of course most arbitrarily generated flat terms will be invalid.

@dcoutts Yes I agree you are right, the generation of InvalidFlat is ad hoc in the sense that it is not covering all possible ways of invalidity but a few fixed ones.

Alternative approach looks fine to me btw do you think that testing arbitrarily generated FlatTerms along with the current setup will be better or its of no use?

I think the currently proposed change here is misleading, and not of a lot of use. I'd accept the PR without the second patch. Or I'd accept the second patch without the bogus property.

Changed the property as per your suggestions!

erikd · 2019-05-16T06:35:53Z

cborg/tests/Tests/FlatTerm.hs

+prop_validFlatTerm :: [TermToken] -> Property
+prop_validFlatTerm ts = property $ case refImpDecode ts of
+  (Just x, Just y)   -> x == y && validFlatTerm ts
+  (Nothing, Nothing) -> True && (not $ validFlatTerm ts)


In that position True does absolutely nothing. You should drop it an just leave the not (validFlatTerm ts).

Basically for all values of x, True && x == x.

You have the logic right here (ie prop_validFlatTerm and refImpDecode) but the structure is a bit weird. Specifically, you have an function that encodes a pair two different ways and returns them as a tuple, when a large chunk of that could just be inlined in the property. Once you inline like that, you probably don't need maybeRdecoded you can just pattern match on an (Maybe a, Either e a) pair.

While i think the above is logically correct, I think that the QC generated [TermToken] data will be invalid for all but the most trivial cases. It would be much nicer if we could manipulate this so that it generated terms that were valid say 80% of the time and invalid 20% of the time. This would give us much better assurance that this property is getting thoroughly tested.

Implemented the requested changes!

erikd · 2019-05-16T06:42:41Z

cborg/tests/Tests/Properties.hs

 --
 prop_toFromFlatTerm :: forall t. Token t => Proxy t -> t -> Bool
 prop_toFromFlatTerm _ x =    

-    liftA2 eq (fn enc) (Right imp) == Right True
+  (deserialiseFromFlatTerm (decodeImp t) . toFlatTerm . encodeImp t) imp  `eq`  imp


You should probably use QuickCheck's === operator there because if the property fails === gives a much better error message.

Here, eq is not directly equivalent to QC's === as eq is defined as eq = eqImp t and eqImp is a method of TypeClass Token for which the test property is written.

Ah, ok. Disregard that comment then.

DeepakKapiswe force-pushed the moretests branch 3 times, most recently from 780d172 to 6dab918 Compare March 25, 2019 07:39

DeepakKapiswe changed the title ~~Add Test for validateFlatTerm function~~ Add test for validateFlatTerm and round trip test for FlatTerm conversions Mar 26, 2019

erikd reviewed Mar 26, 2019

View reviewed changes

cborg/tests/Tests/FlatTerm.hs Outdated Show resolved Hide resolved

DeepakKapiswe force-pushed the moretests branch 4 times, most recently from 4464bf9 to 30eff8a Compare March 27, 2019 07:09

dcoutts requested changes Mar 29, 2019

View reviewed changes

DeepakKapiswe force-pushed the moretests branch 3 times, most recently from e67919c to 7c3e62c Compare May 15, 2019 11:23

erikd reviewed May 16, 2019

View reviewed changes

DeepakKapiswe force-pushed the moretests branch from 7c3e62c to 9c07f06 Compare May 16, 2019 07:10

DeepakKapiswe added 2 commits May 16, 2019 12:43

travis: Update GHC Versions

9c12c4f

Add round trip property tests for FlatTerms

c1cc3d6

DeepakKapiswe force-pushed the moretests branch 2 times, most recently from 5f1fa38 to 761ef0c Compare May 17, 2019 11:52

added test for validFlat along with Ref and Imp Decoders

745af54

DeepakKapiswe force-pushed the moretests branch from 761ef0c to 745af54 Compare May 20, 2019 05:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add test for validateFlatTerm and round trip test for FlatTerm conversions #196

Add test for validateFlatTerm and round trip test for FlatTerm conversions #196

DeepakKapiswe commented Mar 22, 2019 •

edited

Loading

dcoutts left a comment

dcoutts Mar 29, 2019

DeepakKapiswe May 16, 2019

dcoutts Mar 29, 2019

DeepakKapiswe May 16, 2019

dcoutts Mar 29, 2019

DeepakKapiswe May 16, 2019

dcoutts Mar 29, 2019

DeepakKapiswe Apr 1, 2019

dcoutts Apr 1, 2019

DeepakKapiswe May 16, 2019

erikd May 16, 2019 •

edited

Loading

erikd May 16, 2019

erikd May 16, 2019 •

edited

Loading

DeepakKapiswe May 17, 2019

erikd May 16, 2019

DeepakKapiswe May 16, 2019

erikd May 16, 2019



		-- Convert Term to List of Valid TermTokens
		termToValidTermTokens :: Term -> [TermToken]

Add test for validateFlatTerm and round trip test for FlatTerm conversions #196

Are you sure you want to change the base?

Add test for validateFlatTerm and round trip test for FlatTerm conversions #196

Conversation

DeepakKapiswe commented Mar 22, 2019 • edited Loading

dcoutts left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erikd May 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erikd May 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DeepakKapiswe commented Mar 22, 2019 •

edited

Loading

erikd May 16, 2019 •

edited

Loading

erikd May 16, 2019 •

edited

Loading