[darcs-devel] [patch1979] remove obsolete comment (and 7 more)

[Ganesh Sittampalam]

Ganesh Sittampalam <ganesh at earth.li> added the comment:

On 10/02/2020 10:36, Ben Franksen wrote:

> The Shrinkable class has only trivial implementations for
> RepoPatchV1/2/3. It looks as if it is there for sequences of Named
> patches only. If this is true, then why use a class at all and not just
> plain helper functions?

That's a good point. I started out with a minimal implementation and
didn't go back to it. But I think the class could have non-trivial
implementations of at least shrinkAtStart and shrinkAtEnd for
non-conflicts, by shrinking the underlying prim. So I'd be inclined to
keep it as a class but with TODOs about what could be improved later.

> The PropagateShrink class: reading the docs and studying the types did
> not help me understand where the simplifying fixup originally comes
> from. I could finally track this down to shrinkModel, and its
> implementation for V1Model.

I'd better point to that explicitly in the comment.

> What I couldn't find out is how/where
> propagateShrink is actually /used/, other than to implement more
> instances of PropagateShrink. I have tracked usage up to the shrink
> method for WithArbitraryState2 but then I find no uses of that class
> anywhere... and withStateShrinking is also nowhere used.

Oh, sorry. I should have mentioned when sending in these patches that
they are preparatory, in that the new unwind tests use the new
functionality but I haven't submitted those yet.

I've pushed what's still to be submitted to a new branch:
https://hub.darcs.net/ganesh/darcs-unwind-draft-20200211/changes
in particular you can see it being used here:

https://hub.darcs.net/ganesh/darcs-unwind-draft-20200211/patch/e074079d5f0c5ea3843572bf9d73c76720d7bb71


> The MergeableSequence type is quite similar to the old Tree type and
> clearly subsumes it. Can we get rid of Tree now and perhaps rename
> MergeableSequence to MergeableTree? The Arbitrary.Generic module is now
> quite large; does it make sense to move your additions to a separate module?

Those both sound plausible.

> You have two TODO comments in your code. Are you planning to actually do
> something about them? The one in Darcs.Test.Patch.Arbitrary.Named looks
> a bit scary to me.

Thanks for the nudge, I'd sort of forgotten about them as I went.

In D.T.P.Arbitrary.Named, the comment is on 'shrinkInternally' for Named.

    -- TODO this isn't quite right because other patches might
    -- explicitly depend on this one

It's not great for shrinking to produce a bad test case, because it will
obscure the real error and make the debugging experience worse. On the
other hand it's not as bad as actually breaking darcs, and I'm not sure
if it's that likely to matter in practice.

Fixing it might mean reworking it using something like shrinkModel to
push a "name fixup" forwards to any patch that might depend on it. Or I
could perhaps just remove the "shrink pi" case, though it was very handy
in practice for simplifying test cases.

    -- TODO: Possibly @WithStartState (Sealed p)@ could be replaced with
       @Sealed (WithStartState2 p)@.

This one would need some investigation.

I'm happy in principle to look at all of the above items as part of this
review, but obviously it'd take a while so let me know which ones are
most important and which (if any) are ok to leave as "future work".

> One last remark: there are property-based-testing frameworks out there
> that don't require shrinking and automatically find smallest counter
> examples. I am not sure they are up to the task for us, but if not I
> would like to know why.

I think it'd be useful to try a mix of strategies in property-based
testing. The obvious alternative is something exhaustive like lazy
smallcheck, with the obvious possibility that it might be too slow. We
should check if there are libraries that help with overloading over the
different approaches.

__________________________________
Darcs bug tracker <bugs at darcs.net>
<http://bugs.darcs.net/patch1979>
__________________________________