Conformance Test Name Generation is Flawed

[robscott]

What happened:
The test names for HTTP Header Matching in particular are not very helpful. For example, here's the output from one test run:

        --- PASS: TestConformance/HTTPRouteHeaderMatching/9_request_to_/_with_headers_should_go_to_ (0.07s)
        --- PASS: TestConformance/HTTPRouteHeaderMatching/4_request_to_/_with_headers_should_go_to_ (0.07s)
        --- PASS: TestConformance/HTTPRouteHeaderMatching/3_request_to_/_with_headers_should_go_to_ (0.08s)
        --- FAIL: TestConformance/HTTPRouteHeaderMatching/5_request_to_/_with_headers_should_go_to_infra-backend-v1 (30.00s)
        --- FAIL: TestConformance/HTTPRouteHeaderMatching/8_request_to_/_with_headers_should_go_to_infra-backend-v2 (30.00s)
        --- FAIL: TestConformance/HTTPRouteHeaderMatching/0_request_to_/_with_headers_should_go_to_infra-backend-v1 (30.00s)
        --- FAIL: TestConformance/HTTPRouteHeaderMatching/6_request_to_/_with_headers_should_go_to_infra-backend-v1 (30.00s)
        --- FAIL: TestConformance/HTTPRouteHeaderMatching/7_request_to_/_with_headers_should_go_to_infra-backend-v2 (30.00s)
        --- FAIL: TestConformance/HTTPRouteHeaderMatching/2_request_to_/_with_headers_should_go_to_infra-backend-v1 (30.00s)
        --- FAIL: TestConformance/HTTPRouteHeaderMatching/1_request_to_/_with_headers_should_go_to_infra-backend-v2 (30.00s)

What you expected to happen:
Test names to make sense.

How to reproduce it (as minimally and precisely as possible):
Run conformance tests.

Anything else we need to know?:
I think there are at least two fundamental problems here:

1. When Backend is nil we should look to see if a status code has been specified and use that instead: https://github.com/kubernetes-sigs/gateway-api/blob/main/conformance/tests/httproute-header-matching.go#L57
2. We probably want to find a better way to represent "/" in our test case names. As it is, it looks like "2_request_to_" has a subtest named "_with_headers_should_go_to_infra-backend-v1". I'm not sure what the best solution is here, but the following steps could provide some improvement here:
   A. Remove the first "/" character from paths, replace the rest with another character such as "_"
   B. Update test cases to be able to optionally specify their own test name. When specified, it would be used instead of the generated one.

For reference, our current testName function is here:

gateway-api/conformance/tests/httproute-matching-across-routes.go

Lines 118 to 125 in 9537337

	func testName(tc http.ExpectedResponse, i int) string {
	headerStr := ""
	if tc.Request.Headers != nil {
	headerStr = " with headers"
	}
	return fmt.Sprintf("%d request to %s%s%s should go to %s", i, tc.Request.Host, tc.Request.Path, headerStr, tc.Backend)
	}

In addition to the updates suggested above, it should likely be moved to some kind of more clearly shared utils/helpers file as part of these updates.

[abhijit-dev82]

Please assign this issue to me.

[robscott]

/assign @abhijit-dev82

[abhijit-dev82]

@robscott The Headers field being a map, can we use that field to specify a "test case name" by using a specified key name and if not present it will generate the name as done now. And by correcting the path should it look like the below string.

"HTTPRouteHeaderMatching/1_request_to_with_headers_should_go_to_infra-backend-v2"

[robscott]

Hey @abhijit-dev82, I don't think we want to override the "Headers" field like that, instead I'd recommend a new optional string field in the test case itself to specify the name.

If a request is just to "/" we probably shouldn't include a "/" at all, maybe:

HTTPRouteHeaderMatching/1_request_with_headers_should_go_to_infra-backend-v2

If a request is to "/foo" or some other non-root path, maybe it should look like this:

HTTPRouteHeaderMatching/1_request_to_'/foo'_with_headers_should_go_to_infra-backend-v2

I'm not sure if wrapping the path with single quotes like that is actually possible, so open to alternatives if not.

Finally, if a request should not reach a backend and instead should receive some kind of error response, I think we want the name to look something like this:

HTTPRouteHeaderMatching/1_request_with_headers_should_receive_404

[abhijit-dev82]

ok got it, thank you Rob