[darcs-devel] [patch2002] tests: always use prim patches for gener... (and 8 more)

[Ben Franksen]

Ben Franksen <ben.franksen at online.de> added the comment:

Preliminary review.

>   * tests: always use prim patches for generating/shrinking
>   * tests: introduce infrastructure for merge checking
>   * tests: export V1Model(..)
>   * tests: move patch properties into D.T.P.P.Generic
>   * tests: remove unused withStateShrinking

These are all fine, consider reviewed.

>   * tests: generalise hasPrimConstruct, add usesV1Model

You may have expected that I won't like this one overmuch ;-)

I can appreciate the need to move from the Bool to a Dict here and I am
not objecting to that part. However, having both hasPrimConstruct and
hasV1Model smells. Logically these are the same statement i.e. we
require patches based on Prim.V1. Perhaps this is a good time to do
acknowledge that PrimConstruct really cannot be defined usefully for
other prim types... anyway, I don't think this should hold up screening
these patches, just add a TODO marker here so we remember we should
clean this up.

>   * tests: introduce method to identify V1 patch type

Fine.

>   * tests: add withAllSequenceItems

(1) If you recognise that combineTestResults is just mconcat with a
suitable SemiGroup instance I would expect you to replace this function
with mconcat everywhere.
(2) I get a warning about missing implementation of mappend (ghc-8.2). I
think you should define mappend instead of (<>).

>   * introduce concept of unwinding a conflict

For data Unwind you say

-- Note: At the moment we don't require any invariants like minimality
-- for the context.

and further down in consBefore:

    -- Slightly surprisingly, it is possible for a context patch to
commute with the
    -- underlying primitive. If that happens we want to see if we can
eliminate it
    -- by propagating it through the other context ("after" in this case).
    -- "full unwind example 3" fails if this case is omitted, as
(typically) do the standard
    -- 100 iteration QuickCheck tests

I may be wrong, but to me these two statements contradict each other!
Apparently we do have to commute context prims past if that is possible,
which pretty strongly suggests that the context has to be minimal, at
least with respect to commutation.

I am not much surprised that context prims sometimes can be commuted
past for some patch types. For instance, for V2 the context as stored in
the Non inside the conflictor consists of full RepoPatchV2s which could
be conflicted themselves. But we just take their combined effect when we
unwind, so there may well be some parts that commute past.

Can you simplify the examples with a few helper functions? E.g.

mkNamed hash = NamedP (rawPatchInfo "" "" "" ["Ignore-this: "++hash]
path s = AnchoredPath [unsafeMakeName s])

Otherwise: Fine.

__________________________________
Darcs bug tracker <bugs at darcs.net>
<http://bugs.darcs.net/patch2002>
__________________________________
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pEpkey.asc
Type: application/pgp-keys
Size: 4211 bytes
Desc: not available
URL: <http://lists.osuosl.org/pipermail/darcs-devel/attachments/20200224/592831a0/attachment-0001.key>