Flaky test TestGetRoundTripperTLSConfig

[yurishkuro]

jaeger/plugin/metrics/prometheus/metricsstore/reader_test.go

Line 543 in 2ac6aac

assert.Equal(t, "Bearer foo", r.Header.Get("Authorization"))

=== RUN   TestGetRoundTripperTLSConfig/tls_tlsEnabled
    reader_test.go:542: 
        	Error Trace:	/home/runner/work/jaeger/jaeger/plugin/metrics/prometheus/metricsstore/reader_test.go:542
        	            				/opt/hostedtoolcache/go/1.21.0/x64/src/net/http/server.go:2136
        	            				/opt/hostedtoolcache/go/1.21.0/x64/src/net/http/server.go:2938
        	            				/opt/hostedtoolcache/go/1.21.0/x64/src/net/http/server.go:2009
        	            				/opt/hostedtoolcache/go/1.21.0/x64/src/runtime/asm_amd64.s:1650
        	Error:      	Not equal: 
        	            	expected: "***"
        	            	actual  : ""
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1 +1 @@
        	            	-***
        	            	+
        	Test:       	TestGetRoundTripperTLSConfig/tls_tlsEnabled
=== RUN   TestGetRoundTripperTLSConfig/tls_disabled
    reader_test.go:542: 
        	Error Trace:	/home/runner/work/jaeger/jaeger/plugin/metrics/prometheus/metricsstore/reader_test.go:542
        	            				/opt/hostedtoolcache/go/1.21.0/x64/src/net/http/server.go:2136
        	            				/opt/hostedtoolcache/go/1.21.0/x64/src/net/http/server.go:2938
        	            				/opt/hostedtoolcache/go/1.21.0/x64/src/net/http/server.go:2009
        	            				/opt/hostedtoolcache/go/1.21.0/x64/src/runtime/asm_amd64.s:1650
        	Error:      	Not equal: 
        	            	expected: "***"
        	            	actual  : ""
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1 +1 @@
        	            	-***
        	            	+
        	Test:       	TestGetRoundTripperTLSConfig/tls_disabled
--- FAIL: TestGetRoundTripperTLSConfig (0.11s)
    --- FAIL: TestGetRoundTripperTLSConfig/tls_tlsEnabled (0.11s)
    --- FAIL: TestGetRoundTripperTLSConfig/tls_disabled (0.00s)

[CodesbyUnnati]

Hey, I would like to work on this issue.

[CodesbyUnnati]

Hey @yurishkuro, I am unable to replicate this issue. When I'm running make test command or running this unit test it is passing the test case. Please help me replicate this issue. Thanks

[yurishkuro]

Flaky tests are difficult to reproduce reliably. Sometimes you can run it multiple times with -n param and encounter a failure.

[CodesbyUnnati]

I ran the make test -n command multiple times, but can't replicate the issue 😢 How can I fix this issue.

[albertteoh]

I'm a little confused about the test error. The assertion is:

 assert.Equal(t, "Bearer foo", r.Header.Get("Authorization")) 

But the test fails with:

        	Error:      	Not equal: 
        	            	expected: "***"
        	            	actual  : ""

I expected the assertion error to contain something like expected: "Bearer foo", or have I misunderstood something?

@yurishkuro do you have the link to the error run or was it something you encountered from a local run of the test?

[yurishkuro]

@albertteoh it wasn't local, it was from CI. I suspect that the stars are somehow substituted by GitHub runner. If you change the string to foo1 and run the test, the assertion fails with clear text as expected. The fact that actual is shown as empty string suggests that the Authorization header was not sent - I couldn't figure out how that could happen given the code of the test. One option we have is to add t.Logf("request %v", r) to the handler - in the other PR I merged today GitHub was not trying to mask the Authorization header:

    factory_test.go:286: request to fake ES server: &{HEAD / HTTP/1.1 1 1 map[Accept:[application/json] Authorization:[Basic OmZpcnN0IHBhc3N3b3Jk] Content-Type:[application/json] User-Agent:[elastic/6.2.37 (linux-amd64)]] {} <nil> 0 [] false 127.0.0.1:45253 map[] map[] <nil> map[] 127.0.0.1:59670 / <nil> <nil> <nil> 0xc000536b40}

[albertteoh]

Yeah, that's a good idea. I'll put that in.

I'm wondering if httptest.NewServer will strictly guarantee isolation to this test. Just speculating if another test or another process is coincidentally hitting that same test server URL so the assertion is prematurely invoked before (or after?) the actual test makes the request.

As such, I wonder if it's worthwhile changing the test to have a boolean variable outside and setting this to true only if it's received a request where the auth header is Bearer foo? It has the added benefit of actually asserting we received the request. The current test can still pass even though we didn't receive that request.

So something like:

			var gotExpectedRequest bool
			server := httptest.NewServer(
				http.HandlerFunc(
					func(w http.ResponseWriter, r *http.Request) {
						if r.Header.Get("Authorization") == "Bearer foo" {
							gotExpectedRequest = true
						}
					},
				),
			)
...
			assert.True(t, gotExpectedRequest)

[yurishkuro]

I believe each test server runs on a random port, so crossover should not happen.

I agree that it's better to have the handler store the data somewhere and then assert on that data in the main test. Example: https://github.com/jaegertracing/jaeger/pull/4342/files#diff-fb005680ccab1461b654fe14dcb0a53eb862af4f9bfc15b279731e287083b5daR285 - this one is a bit more complicated because basic auth is encoded and I didn't want to compare values in the encoded form (not informative if assertion fails). Note that you should use synchronized storage var, otherwise your test may cause race condition detector to flag it.