Issue #3349 - enable -shuffle=on in tests

[rbroggi]

Which problem is this PR solving?

• Resolves Enable test shuffle for more robust test suite #3349

Short description of the changes

• Enabling shuffling in tests

[codecov]

Codecov Report

Merging #3350 (14ed70d) into master (514e72b) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #3350   +/-   ##
=======================================
  Coverage   96.47%   96.47%           
=======================================
  Files         259      259           
  Lines       15178    15178           
=======================================
  Hits        14643    14643           
- Misses        451      452    +1     
+ Partials       84       83    -1     

Impacted Files	Coverage Δ
pkg/config/tlscfg/cert_watcher.go	92.63% <0.00%> (-2.11%)	⬇️
cmd/query/app/static_handler.go	97.00% <0.00%> (-0.60%)	⬇️
cmd/query/app/server.go	95.58% <0.00%> (+1.47%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 514e72b...14ed70d. Read the comment docs.

[rbroggi]

One of the nice features of shuffle flag is that whenever you run your tests with it on the command-line will print to stdout the seed of the shuffling so that if flakiness is experienced, it's easily reproduceable:

e.g.:

go test -v -race -shuffle=on ./... 
-test.shuffle 1635607575548663755

you can use the test shuffle to reproduce the issue:

go test -v -race -shuffle=1635607575548663755 ./... 

[yurishkuro]

Generally I am not aware of inter-test dependencies, we always tried to keep tests independent. I see that TestAggregateThroughput is failing, which is a relatively recent test, some side-effect dependency might have sneaked in & we should fix it.

[rbroggi]

It's very likely that this global variable is the source of the flakiness as it's not read-only as it should therefore impacting also this case.

[yurishkuro]

I am a bit concerned by this approach. The input to p.aggregateThroughput is supposed to be treated as read-only by that function. If it's not, that might be the bug / source of the test dependencies, not the fault of the test. The testThroughputs is not a global variable, it's logically a constant. By using a function instead of the same value we simply obscure the bug elsewhere.

[rbroggi]

Let me see if I understand your point:

since method p.aggregateThroughput is supposed to treat its inputs as read-only, you think that my approach could hide a potential bug in the p.aggregateThroughput that could potentially mutate the input?

If this is what you mean I think my change is still valid because it's enforcing independence between tests. And what we could have is a specific test that tests for p.aggregateThroughput immutability. Currently the thing that is actually mutating the content of the global variable is a test on its body (see in master TestRealisticRunCalculationLoop in processor_test:445)

[rbroggi]

I have created the following test case and to your point it indeed fail (which means that the method is indeed mutating the arguments):

func TestAggregateThroughputInputsImmutability(t *testing.T) {
	p := &Processor{}
	in := testThroughputs()
	_ = p.aggregateThroughput(in)
	assert.Equal(t, in, testThroughputs())
}

[rbroggi]

And if indeed it's supposed not to mutate the arguments the issue is exactly here:

plugin/sampling/strategystore/adaptive/processor.go:313

Pointer semantics 😋

[rbroggi]

Generally I am not aware of inter-test dependencies, we always tried to keep tests independent. I see that TestAggregateThroughput is failing, which is a relatively recent test, some side-effect dependency might have sneaked in & we should fix it.

Found the source of the bug: global variable 😌

My approach in the test was to change all the global variables of the file to methods which return the same value, this way we cannot share state across tests (even in future developments)

[rbroggi]

Update on this one:

TestHotReloadUIConfigTempFile fails now. I think shuffle is already paying some dividends spoting flakiness.
It will probably take a while for me to troubleshoot those issues as I'm not yet familiar with the code.

Another intriguing thing is that the all-in-one tests fail but only for the debug image, the release one works fine 🤔

[rbroggi]

if you run this test method on master branch it will fail

[yurishkuro]

+1

Given that the CI still fails due to some other issues, do you want to merge this PR without the -shuffle=on in the Makefile?

[rbroggi]

I think it will profitable to keep the interesting commenting history therefore:

#3360

[rbroggi]

This is probably a bug (unintended pointer copy makes so that the input argument throughputs is changed during this method execution). Could you please @yurishkuro / @albertteoh confirm whether this method is supposed to be immutable on its arguments?

[rbroggi]

bug hunted thanks to shuffle 😋

[stale]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. You may re-open it if you need more time.

[stale]

This pull request has been automatically closed due to inactivity. You may re-open it if you need more time. We really appreciate your contribution and we are sorry that this has not been completed.