go-test / deep

Golang deep variable equality test that returns human-readable differences
MIT License
743 stars 54 forks source link

default Depth #21

Closed sentient closed 5 years ago

sentient commented 5 years ago

Not sure if there is a limitation on the depth with the default package

reflect.DeepEqual(got, tt.want)

but changing the code out with just

if diff := deep.Equal(got, tt.want); diff != nil {
    t.Error(diff)
}

is kind of dangerous. The dept default is set to just 10.
I had to set it to

deep.MaxDepth = 100

to find my issue.

Maybe generate a warning when the struct is deeper and you stop checking? Or increase the default much higher ?

Probably the best is

if !reflect.DeepEqual(got, tt.want) {
        //try a friendly deep error message
    if diff := deep.Equal(got, tt.want); diff != nil {
        t.Error(diff)
    } else {
        t.Errorf("deepEqual failed \n%v\nbut I  want \n%v", got, tt.want)   
    }
}
sentient commented 5 years ago

BTW: Still thumbs up on this code

phillipao commented 5 years ago

Warning is not sufficient, because tests are typically run in an automation context, where no one is looking at the logs. When the depth restriction is triggered, the comparison needs to fail, not pass. Particularly with 10 as the limit. It's very easy to trigger. But even with a higher limit, it's not good to fail open in this way. I wonder why this depth limit is necessary at all. Is it to guard against cycles? A real cycle detection mechanism would be better.

daniel-nichter commented 5 years ago

reflect.DeepEqual has no limit; it only has:

// if depth > 10 { panic("deepValueEqual") }    // for debugging

So I'll effectively remove the limit from this pkg because no arbitrary number will be universally correct.

I'll make default value zero and mean "no limit". So if users still want a sanity check, they can set MaxDepth to whatever value they want greater than zero.

daniel-nichter commented 5 years ago

Merged ^. If that doesn't address issue, let me know/re-open this issue. Thanks!