suite: fix deadlock in suite.Require()/Assert()

[arjunmahishi]

As pointed out in issue #1520, if the suite is not initialised properly (by calling the Run function), then calling suite.Require() or suite.Assert() will result in a deadlock.

Changes

This commit fixes that by panicking if the suite is not initialised properly. This is justified because the suite is intended to be triggered in the right way. If the user does not do that, this panic will nudge them in the right direction.

It has to be a panic because, at this point, we don't have access to any testing.T context to gracefully call a t.Fail(). Also, these two functions are not expected to return an error.

Related issues

Fixes #1520

[arjunmahishi]

This is a breaking change.

How? This would have never worked in the previous version right?

You can fix the deadlock by making two versions of the T method

But just like suite.require, suite.t would also not be initialised without calling Run. So, the user would run into a nil pointer deref panic.

                                          ┌─────────────────────────────────────────┐ 
                                          │func (suite *Suite) SetT(t *testing.T) { │ 
                                          │  suite.mu.Lock()                        │
┌─────┐        ┌─────────────┐            │  defer suite.mu.Unlock()                │
│Run()├───────►│suite.SetT(t)├───────────►│  suite.t = t                            │
└─────┘        └─────────────┘            │  suite.Assertions = assert.New(t)       │
                                          │  suite.require = require.New(t)         │
                                          │}                                        │ 
                                          └─────────────────────────────────────────┘ 

[brackendawson]

At v1.0 (which is difficult to require from a modern module) SetT exists and does not initialise suite.Suite.Require, and the code path we are modifying was useful, it sucessfully returned a functioning Require to this contrived example:

package kata_test

import (
    "os"
    "testing"

    "github.com/stretchr/testify/suite"
)

type mySuite struct {
    suite.Suite
}

func (suite *mySuite) TestSomething() {
    require := suite.Require()
    require.True(true)
}

func TestSuite(t *testing.T) {
    m := &mySuite{}
    m.SetT(t)
    f, err := os.Create("testfile")
    m.Require().NoError(err)
    defer f.Close()
    suite.Run(t, m)
}

This code works today because SetT also initialises both suite.Suite.Assert and suite.Suite.Require, meaning we don't traverse the code we're modifying, which has at various times been a nil pointer dereference, a deadlock, or now a deliberate panic.

So I think you are right, I can't contrive an example that is broken by this change, it's not a breaking change.

Perhaps though the panic message should encompass something like: "Require must not be called before Run or SetT".

[arjunmahishi]

Ohh got it. Thanks for the context! Are you on board with panicking though? (I've changed the message)