Initial state of slices are not held for reporting

[ymotongpoo]

First of all, I do appreciate all your work on this project. This library is fantastic. I was playing with gopter and found that the property doesn't seem to hold the original state of the sample for reporting, which gives confusing results.

First, I prepared a small target function named biggest, which is expected to return the biggest value in the given slice of int.

func biggest(ns []int) (int, error) {
    if len(ns) == 0 {
        return 0, fmt.Errorf("slice must have at least one element")
    }
    return ns[0], nil
}

As the PBT of this function, I wrote the following test:

func TestBiggest(t *testing.T) {
    properties := gopter.NewProperties(nil)
    properties.Property("Non-zero length small int slice", prop.ForAll(
        func(ns []int) bool {
            result, _ := biggest(ns)
            sort.Slice(ns, func(i, j int) bool {
                return ns[i] > ns[j]
            })
            return result == ns[0]
        },
        gen.SliceOf(gen.IntRange(0, 20),
            reflect.TypeOf(int(0))).
            SuchThat(func(v interface{}) bool {
                return len(v.([]int)) > 0
            }),
    ))
    properties.TestingRun(t)
}

The result of this test was:

! Non-zero length small int slice: Falsified after 0 passed tests.
ARG_0: [13 0]
ARG_0_ORIGINAL (1 shrinks): [14 13]
Elapsed time: 164.792µs
--- FAIL: TestBiggest (0.00s)
    /home/ymotongpoo/personal/pbt/sample/properties.go:57: failed with initial seed: 1602330526004760703
FAIL

But these arguments, both the original and the shrunk, should pass the test. Though I haven't dug into the implementation of the default shrinker and reporting, I assumed that the shrinker doesn't hold the original state of the sample. I changed the test code a bit to keep the original state of sample and it returned the expected report.

func TestBiggest(t *testing.T) {
    properties := gopter.NewProperties(nil)
    properties.Property("Non-zero length small int slice", prop.ForAll(
        func(ns []int) bool {
            nns := make([]int, len(ns))
            copy(nns, ns)
            result, _ := biggest(nns)
            sort.Slice(nns, func(i, j int) bool {
                return nns[i] > nns[j]
            })
            return result == nns[0]
        },
        gen.SliceOf(gen.IntRange(0, 20),
            reflect.TypeOf(int(0))).
            SuchThat(func(v interface{}) bool {
                return len(v.([]int)) > 0
            }),
    ))
    properties.TestingRun(t)
}

This test made the following report that makes sense to me:

! Non-zero length small int slice: Falsified after 0 passed tests.
ARG_0: [0 1]
ARG_0_ORIGINAL (6 shrinks): [3 19]
Elapsed time: 245.025µs
--- FAIL: TestBiggest (0.00s)
    /home/ymotongpoo/personal/pbt/sample/properties.go:57: failed with initial seed: 1602333441977570032
FAIL

[untoldwind]

When creating an array/slice the implementation indeed does not keep a copy, so if the array is changed by the test-function the report will be messed up.

Since there is a lot of reflection involved at this point, this is actually no so easy to fix. I'll take a look into it.

[untoldwind]

FYI: I'm currently experimenting with creating deep clones for the function parameters: https://github.com/leanovate/gopter/tree/deep_clone

It already solves your problem, but I'm not sure about the performance impact.

[untoldwind]

Here is an alternative solution: https://github.com/leanovate/gopter/tree/serialize_early

In this case the output is:

     ! Non-zero length small int slice: Falsified after 1 passed tests.
     ARG_0: [0 5 0]
     ARG_0_ORIGINAL (1 shrinks): [0 7 5]

I.e. the shrinker still goes slightly haywire because the slice is modified by the test function, but at least the arguments are reported correctly.

I'd rather prefer this solution as it has less impact on the existing runtime. Deep cloning of arbitrary interfaces in go is pretty messy and might cause a lot of trouble.

[ymotongpoo]

Thank you, @untoldwind for the experiments and sharing the result from them. The latter option, i.e. serialization sounds better for me as well, especially in the case of complex data structure as you say. I look forward to having the feature earlier.

[untoldwind]

I merged the later branch to master. You can give it a try.

[ymotongpoo]

Awesome! Thank you!

[untoldwind]

I think it is ok to close this ...