New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

proposal: testing: add shuffle flag #28592

Open
cristaloleg opened this Issue Nov 4, 2018 · 4 comments

Comments

Projects
None yet
4 participants
@cristaloleg

cristaloleg commented Nov 4, 2018

This is a revive of #10655

Motivation

Consider the following code & corresponding tests:

package pkg

// build and cache regexp, reuse between clients
var re *regexp.Regexp

type Client struct {
	// ...
}

func NewClient(pattern string) *Client {
	if re == nil {
		re = regexp.MustCompile(pattern)
	}
	return &Client{
		// ...
	}
}

func (c *Client) HasPattern(s string) error {
	if !re.MatchString(s) {
		return errors.New("meh, incorrect param")
	}
	return nil
}
package pkg

func TestFoo(t *testing.T) {
	s := NewClient("^foo.*")
	err := s.HasPattern("foo123")
	if err != nil {
		t.Errorf("expected to pass")
	}
}

func TestFooBar(t *testing.T) {
	s := NewClient("^foobar.*")
	err := s.HasPattern("foobar1123")
	if err != nil {
		t.Errorf("expected to pass")
	}
}

Those tests pass, everything looks fine, but they're order dependent. Running them in another order will fail.

To prevent such hidden and hard to debug mistakes we need to make the order of test random for each test build.

Current workarounds

  1. Manual ordering of tests.
  2. Boilerplate code to specify the order of tests.
  3. For table-driven tests we can use a map instead of slice, ex:
func TestSomething(t *testing.T) {
	testCases := map[name]struct{
		a,b int64
		res int64
	}{
		“simple case”: {1, 2, 3},
		“less simple”: {3, 3, 23},
	}

	// due to behaviour of map test cases will be 
	// in a different order for each run
	// but this solution is limited and not suitable for test funcs
	// aka `func TestXXX(t *testing.T)`
	for name, tc := range testCases {
		t.Logf(“test: %s”, name)

		res := foo(tc.a, tc.b)
		if res != tc.res {
			t.Errorf(“want %v, got %v, res, tc.res)
		}
	}
}

Possible solution

We need to specify a test run to execute tests with a random/desired order:

  1. -shuffle to run tests with a random order (used random seed may or should be printed to output depending or not on a -v flag).
  2. -seed <int> to specify a user-defined seed to have an ability repeat a failed build.

Open questions

  1. Should we randomize order of benchmarks and examples? (looks like not)
  2. This makes test runs 'flaky' but this helps to reveal possible implementation caveats.
  3. This might not happen for 1.12 'cause proposal is submitted too late.

PS. if/when it will be accepted - will be happy to work on it.

@gopherbot gopherbot added this to the Proposal milestone Nov 4, 2018

@gopherbot gopherbot added the Proposal label Nov 4, 2018

@mvdan

This comment has been minimized.

Member

mvdan commented Nov 5, 2018

Interesting problem - reminds me of go test -race a bit. I'm a bit worried that every repository will now be expected to run three test commands on its CI; go test; go test -race; go test -shuffle.

On the other hand, making go test randomize the order could make its output non-deterministic.

I wonder if this is something we could enable when one runs go test ./.... That is, since a package's test output and results are printed all at once, we could run the tests in random order and then sort the results before printing them. That would be ideal in my opinion, since every existing user would benefit and we wouldn't need an extra flag.

That suggestion wouldn't apply to go test or go test -v, since those do need to run the tests in order - the results are printed as they happen.

@cristaloleg

This comment has been minimized.

cristaloleg commented Nov 5, 2018

That is, since a package's test output and results are printed all at once

Might be a problem with #24929 😉

and then sort the results before printing them

But how we can know an order that triggered a test run failure? Sort only success run?

@mvdan

This comment has been minimized.

Member

mvdan commented Nov 5, 2018

Might be a problem with #24929

Oh, I thought that proposal would affect go test -v, not go test -v ./.... If it affects both, then you're right.

But how we can know an order that triggered a test run failure? Sort only success run?

Potentially. Or one could take the usual approach like with any other test flake - run the tests over and over again with -count or stress until we find the failure again or we're convinced that it's fixed.

@rsc

This comment has been minimized.

Contributor

rsc commented Dec 12, 2018

More command-line flags seems problematic.
What if instead we added a method on testing.M so that TestMain could call m.Shuffle(time.Now().UnixNano()) in tests that wanted to opt in to randomized execution?
Those tests could also define their own seed flags instead of having to add one to all tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment