stretchr / testify

A toolkit with common assertions and mocks that plays nicely with the standard library
MIT License
22.5k stars 1.56k forks source link

Regexp accepts interface{} #1585

Closed kevinburkesegment closed 2 months ago

kevinburkesegment commented 2 months ago

The signature for assert.Regexp (and associated methods) is:

func Regexp(t TestingT, rx interface{}, str interface{}, msgAndArgs ...interface{}) bool {

However, these methods immediately call matchRegexp, which is this:

func matchRegexp(rx interface{}, str interface{}) bool {

    var r *regexp.Regexp
    if rr, ok := rx.(*regexp.Regexp); ok {
        r = rr
    } else {
        r = regexp.MustCompile(fmt.Sprint(rx))
    }

    return (r.FindStringIndex(fmt.Sprint(str)) != nil)

}

Given this - wouldn't it make more sense for the signature to just be *regexp.Regexp and string ? The latter cast would likely help prevent unexpected behavior around the fmt.Sprint call.

dolmen commented 2 months ago

Changing the signature of assert.Regexp, assert.NotRegexp (...) with stricter types would break compatibility. At this point detecting usage errors can only be made via a linter like testifylint.

ccoVeille commented 2 months ago

Changing the signature of assert.Regexp, assert.NotRegexp (...) with stricter types would break compatibility. At this point detecting usage errors can only be made via a linter like testifylint.

I agree with you about the compatibility issue if it was changed now, but what about changing the signature for testify v2?

Would it be acceptable?

ccoVeille commented 2 months ago

For record, I noticed by myself the way to solve this in testify is way more complex than I thought

I'm talking about it here

https://github.com/Antonboom/testifylint/issues/81#issuecomment-2060642389