Flaky E2E test: e2e/TestDomain - create domain with TLS

[dsimansk]

The e2e tests are intermittently failing on e2e/TestDomain. In a specific https test case. When the URL should contain https but is empty. It's most likely a timing issue with empty value being checked prematurely.

There's a wait loop to wait for URL to be populated before asserting it. It might be not working as expected.
https://github.com/knative/client/blob/2d0415a7951b99130ccb514fa0e3c5343338225c/test/e2e/domain_mapping_test.go#L136C1-L144C3

Error log:
https://prow.knative.dev/view/gs/knative-prow/logs/continuous_client_main_periodic/1710944161479266304#

/kind good-first-issue

[xiangpingjiang]

/assign

[dsimansk]

The original issue is not fixed. We need to further investigate & verify the wait loop is working as expected and maybe fix ti.

@xiangpingjiang I'll remove your previous assignment. But feel free to assign again and do a second pass if you are still interested.

[xiangpingjiang]

The original issue is not fixed. We need to further investigate & verify the wait loop is working as expected and maybe fix ti.

@xiangpingjiang I'll remove your previous assignment. But feel free to assign again and do a second pass if you are still interested.

@dsimansk
Did you find that e2e/TestDomain not pass after #1888 merged?

[dsimansk]

The original issue is not fixed. We need to further investigate & verify the wait loop is working as expected and maybe fix ti.
@xiangpingjiang I'll remove your previous assignment. But feel free to assign again and do a second pass if you are still interested.

@dsimansk Did you find that e2e/TestDomain not pass after #1888 merged?

Yes. E.g.: https://prow.knative.dev/view/gs/knative-prow/pr-logs/pull/knative_client/1901/integration-tests-latest-release_client_main/1729841947746504704

[xiangpingjiang]

/assign

[dsimansk]

The suspicious part in test log is the following. See age: field is 1s, URL is still empty, but the test is already failed with missing string error.

There's still possibility to a short timeout (e.g. 3 - 5s) between create and verify operations. But I'd rather check first if the wait loop is working as expected.

domain_mapping_test.go:153: assertion failed: 
        Actual output: Name:       newdomain.com
        Namespace:  kne2etests21
        Age:        1s
        
        URL:  
        
        Reference:     
          APIVersion:  serving.knative.dev/v1
          Kind:        Service
          Name:        hello
        
        Conditions:  
          OK TYPE    AGE REASON
        
        Missing strings: https://newdomain.com/

[Ipppoooo]

It could be a timing related issue. This may work better for the for loop

func domainDescribe(r *test.KnRunResultCollector, domainName string, tls bool) {
	k := test.NewKubectl(r.KnTest().Kn().Namespace())

	// Wait for Domain Mapping URL to be populated
	var url string
	timeout := time.After(30 * time.Second) // Adjust the timeout as needed

	for {
		select {
		case <-time.After(time.Second):
			out, err := k.Run("get", "domainmapping", domainName, "-o=jsonpath='{.status.url}'")
			assert.NilError(r.T(), err)

			if len(out) > 0 {
				url = out
				break
			}
		case <-timeout:
			assert.Fail(r.T(), "Timed out waiting for the domain mapping URL to be populated")
			return
		}
	}

	out := r.KnTest().Kn().Run("domain", "describe", domainName)
	r.AssertNoError(out)

	if tls {
		url = "https://" + domainName
	} else {
		url = "http://" + domainName
	}

	assert.Assert(r.T(), util.ContainsAll(out.Stdout, "Name", "Namespace", "URL", "Service", url))
}

By introducing this more robust waiting mechanism, you can mitigate timing issues and reduce the likelihood of intermittent test failures.

[rhuss]

Thanks @Ipppoooo , i think its worth a try. Maybe don't retry every second as this will cause 30 calls if this is a non-recoverable error. Maybe use e.g. 5seconds or an exponential backoff. Fancy to send in a PR ?

[xiangpingjiang]

hello @rhuss
I think the reason maybe the break condition in for loop is len(out) > 0, but the assert condition is util.ContainsAll(out.Stdout, "Name", "Namespace", "URL", "Service", url).

It's different conditions.

What do you think?