From 3efa7bdcc27b137d072d4b04e92478064c6086b8 Mon Sep 17 00:00:00 2001 From: Matt Moyer Date: Wed, 16 Jun 2021 17:51:23 -0500 Subject: [PATCH] Improve our integration test "Eventually" assertions. This fixes some rare test flakes caused by a data race inherent in the way we use `assert.Eventually()` with extra variables for followup assertions. This function is tricky to use correctly because it runs the passed function in a separate goroutine, and you have no guarantee that any shared variables are in a coherent state when the `assert.Eventually()` call returns. Even if you add manual mutexes, it's tricky to get the semantics right. This has been a recurring pain point and the cause of several test flakes. This change introduces a new `library.RequireEventually()` that works by internally constructing a per-loop `*require.Assertions` and running everything on a single goroutine (using `wait.PollImmediate()`). This makes it very easy to write eventual assertions. Signed-off-by: Matt Moyer --- test/integration/category_test.go | 33 +----- .../concierge_api_serving_certs_test.go | 45 +++----- test/integration/concierge_client_test.go | 29 ++--- .../concierge_credentialrequest_test.go | 32 +++--- .../concierge_impersonation_proxy_test.go | 92 +++++++++------- test/integration/ldap_client_test.go | 23 +--- test/integration/supervisor_discovery_test.go | 72 ++++++------ test/integration/supervisor_login_test.go | 17 ++- test/integration/supervisor_secrets_test.go | 18 ++- ...ervisor_storage_garbage_collection_test.go | 24 ++-- test/library/access.go | 64 ++++------- test/library/assertions.go | 104 ++++++++++++++++++ test/library/browsertest/browsertest.go | 20 ++-- test/library/client.go | 68 +++++------- 14 files changed, 318 insertions(+), 323 deletions(-) diff --git a/test/integration/category_test.go b/test/integration/category_test.go index 253d0d62..d307e0d3 100644 --- a/test/integration/category_test.go +++ b/test/integration/category_test.go @@ -7,11 +7,9 @@ import ( "bytes" "os/exec" "strings" - "sync" "testing" "time" - "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "go.pinniped.dev/test/library" @@ -19,40 +17,15 @@ import ( func runTestKubectlCommand(t *testing.T, args ...string) (string, string) { t.Helper() - - var lock sync.Mutex var stdOut, stdErr bytes.Buffer - var err error - start := time.Now() - attempts := 0 - if !assert.Eventually(t, func() bool { - lock.Lock() - defer lock.Unlock() - attempts++ + library.RequireEventually(t, func(requireEventually *require.Assertions) { stdOut.Reset() stdErr.Reset() cmd := exec.Command("kubectl", args...) cmd.Stdout = &stdOut cmd.Stderr = &stdErr - err = cmd.Run() - return err == nil - }, - 120*time.Second, - 200*time.Millisecond, - ) { - lock.Lock() - defer lock.Unlock() - t.Logf( - "never ran %q successfully even after %d attempts (%s)", - "kubectl "+strings.Join(args, " "), - attempts, - time.Since(start).Round(time.Second), - ) - t.Logf("last error: %v", err) - t.Logf("stdout:\n%s\n", stdOut.String()) - t.Logf("stderr:\n%s\n", stdErr.String()) - t.FailNow() - } + requireEventually.NoError(cmd.Run()) + }, 120*time.Second, 200*time.Millisecond) return stdOut.String(), stdErr.String() } diff --git a/test/integration/concierge_api_serving_certs_test.go b/test/integration/concierge_api_serving_certs_test.go index c3e32b56..4e5d9f3d 100644 --- a/test/integration/concierge_api_serving_certs_test.go +++ b/test/integration/concierge_api_serving_certs_test.go @@ -4,12 +4,10 @@ package integration import ( - "bytes" "context" "testing" "time" - "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/kubernetes" @@ -109,12 +107,11 @@ func TestAPIServingCertificateAutoCreationAndRotation(t *testing.T) { require.NoError(t, test.forceRotation(ctx, kubeClient, env.ConciergeNamespace)) // Expect that the Secret comes back right away with newly minted certs. - secretIsRegenerated := func() bool { + library.RequireEventually(t, func(requireEventually *require.Assertions) { + var err error secret, err = kubeClient.CoreV1().Secrets(env.ConciergeNamespace).Get(ctx, defaultServingCertResourceName, metav1.GetOptions{}) - return err == nil - } - assert.Eventually(t, secretIsRegenerated, 10*time.Second, 250*time.Millisecond) - require.NoError(t, err) // prints out the error and stops the test in case of failure + requireEventually.NoError(err) + }, 10*time.Second, 250*time.Millisecond) regeneratedCACert := secret.Data["caCertificate"] regeneratedPrivateKey := secret.Data["tlsPrivateKey"] regeneratedCertChain := secret.Data["tlsCertificateChain"] @@ -130,18 +127,10 @@ func TestAPIServingCertificateAutoCreationAndRotation(t *testing.T) { require.Equal(t, env.ConciergeAppName, secret.Labels["app"]) // Expect that the APIService was also updated with the new CA. - require.Eventually(t, func() bool { + library.RequireEventually(t, func(requireEventually *require.Assertions) { apiService, err := aggregatedClient.ApiregistrationV1().APIServices().Get(ctx, apiServiceName, metav1.GetOptions{}) - if err != nil { - t.Logf("get for APIService %q returned error %v", apiServiceName, err) - return false - } - if !bytes.Equal(regeneratedCACert, apiService.Spec.CABundle) { - t.Logf("CA bundle in APIService %q does not yet have the expected value", apiServiceName) - return false - } - t.Logf("found that APIService %q was updated to expected CA certificate", apiServiceName) - return true + requireEventually.NoErrorf(err, "get for APIService %q returned error", apiServiceName) + requireEventually.Equalf(regeneratedCACert, apiService.Spec.CABundle, "CA bundle in APIService %q does not yet have the expected value", apiServiceName) }, 10*time.Second, 250*time.Millisecond, "never saw CA certificate rotate to expected value") // Check that we can still make requests to the aggregated API through the kube API server, @@ -149,25 +138,19 @@ func TestAPIServingCertificateAutoCreationAndRotation(t *testing.T) { // so this is effectively checking that the aggregated API server is using these new certs. // We ensure that 10 straight requests succeed so that we filter out false positives where a single // pod has rotated their cert, but not the other ones sitting behind the service. - aggregatedAPIWorking := func() bool { + // + // our code changes all the certs immediately thus this should be healthy fairly quickly + // if this starts flaking, check for bugs in our dynamiccertificates.Notifier implementation + library.RequireEventually(t, func(requireEventually *require.Assertions) { for i := 0; i < 10; i++ { - _, err = conciergeClient.LoginV1alpha1().TokenCredentialRequests().Create(ctx, &loginv1alpha1.TokenCredentialRequest{ + _, err := conciergeClient.LoginV1alpha1().TokenCredentialRequests().Create(ctx, &loginv1alpha1.TokenCredentialRequest{ TypeMeta: metav1.TypeMeta{}, ObjectMeta: metav1.ObjectMeta{}, Spec: loginv1alpha1.TokenCredentialRequestSpec{Token: "not a good token", Authenticator: testWebhook}, }, metav1.CreateOptions{}) - if err != nil { - break - } + requireEventually.NoError(err, "dynamiccertificates.Notifier broken?") } - // Should have got a success response with an error message inside it complaining about the token value. - return err == nil - } - - // our code changes all the certs immediately thus this should be healthy fairly quickly - // if this starts flaking, check for bugs in our dynamiccertificates.Notifier implementation - assert.Eventually(t, aggregatedAPIWorking, 30*time.Second, 250*time.Millisecond) - require.NoError(t, err, "dynamiccertificates.Notifier broken?") // prints out the error and stops the test in case of failure + }, 30*time.Second, 250*time.Millisecond) }) } } diff --git a/test/integration/concierge_client_test.go b/test/integration/concierge_client_test.go index 175daee5..f26db73c 100644 --- a/test/integration/concierge_client_test.go +++ b/test/integration/concierge_client_test.go @@ -9,9 +9,7 @@ import ( "testing" "time" - "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - clientauthenticationv1beta1 "k8s.io/client-go/pkg/apis/clientauthentication/v1beta1" "go.pinniped.dev/internal/here" "go.pinniped.dev/pkg/conciergeclient" @@ -77,20 +75,17 @@ func TestClient(t *testing.T) { ) require.NoError(t, err) - var resp *clientauthenticationv1beta1.ExecCredential - assert.Eventually(t, func() bool { - resp, err = client.ExchangeToken(ctx, env.TestUser.Token) - return err == nil + library.RequireEventually(t, func(requireEventually *require.Assertions) { + resp, err := client.ExchangeToken(ctx, env.TestUser.Token) + requireEventually.NoError(err) + requireEventually.NotNil(resp.Status.ExpirationTimestamp) + requireEventually.InDelta(5*time.Minute, time.Until(resp.Status.ExpirationTimestamp.Time), float64(time.Minute)) + + // Create a client using the certificate and key returned by the token exchange. + validClient := library.NewClientsetWithCertAndKey(t, resp.Status.ClientCertificateData, resp.Status.ClientKeyData) + + // Make a version request, which should succeed even without any authorization. + _, err = validClient.Discovery().ServerVersion() + requireEventually.NoError(err) }, 10*time.Second, 500*time.Millisecond) - require.NoError(t, err) - - require.NotNil(t, resp.Status.ExpirationTimestamp) - require.InDelta(t, 5*time.Minute, time.Until(resp.Status.ExpirationTimestamp.Time), float64(time.Minute)) - - // Create a client using the certificate and key returned by the token exchange. - validClient := library.NewClientsetWithCertAndKey(t, resp.Status.ClientCertificateData, resp.Status.ClientKeyData) - - // Make a version request, which should succeed even without any authorization. - _, err = validClient.Discovery().ServerVersion() - require.NoError(t, err) } diff --git a/test/integration/concierge_credentialrequest_test.go b/test/integration/concierge_credentialrequest_test.go index 8931859d..83d6ed74 100644 --- a/test/integration/concierge_credentialrequest_test.go +++ b/test/integration/concierge_credentialrequest_test.go @@ -10,7 +10,6 @@ import ( "testing" "time" - "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" jwtpkg "gopkg.in/square/go-jose.v2/jwt" corev1 "k8s.io/api/core/v1" @@ -88,26 +87,25 @@ func TestSuccessfulCredentialRequest(t *testing.T) { token, username, groups := test.token(t) var response *loginv1alpha1.TokenCredentialRequest - successfulResponse := func() bool { + library.RequireEventually(t, func(requireEventually *require.Assertions) { var err error response, err = library.CreateTokenCredentialRequest(ctx, t, loginv1alpha1.TokenCredentialRequestSpec{Token: token, Authenticator: authenticator}, ) - require.NoError(t, err, "the request should never fail at the HTTP level") - return response.Status.Credential != nil - } - assert.Eventually(t, successfulResponse, 10*time.Second, 500*time.Millisecond) - require.NotNil(t, response) - require.Emptyf(t, response.Status.Message, "value is: %q", safeDerefStringPtr(response.Status.Message)) - require.NotNil(t, response.Status.Credential) - require.Empty(t, response.Spec) - require.Empty(t, response.Status.Credential.Token) - require.NotEmpty(t, response.Status.Credential.ClientCertificateData) - require.Equal(t, username, getCommonName(t, response.Status.Credential.ClientCertificateData)) - require.ElementsMatch(t, groups, getOrganizations(t, response.Status.Credential.ClientCertificateData)) - require.NotEmpty(t, response.Status.Credential.ClientKeyData) - require.NotNil(t, response.Status.Credential.ExpirationTimestamp) - require.InDelta(t, 5*time.Minute, time.Until(response.Status.Credential.ExpirationTimestamp.Time), float64(time.Minute)) + requireEventually.NoError(err, "the request should never fail at the HTTP level") + requireEventually.NotNil(response) + requireEventually.NotNil(response.Status.Credential, "the response should contain a credential") + requireEventually.Emptyf(response.Status.Message, "value is: %q", safeDerefStringPtr(response.Status.Message)) + requireEventually.NotNil(response.Status.Credential) + requireEventually.Empty(response.Spec) + requireEventually.Empty(response.Status.Credential.Token) + requireEventually.NotEmpty(response.Status.Credential.ClientCertificateData) + requireEventually.Equal(username, getCommonName(t, response.Status.Credential.ClientCertificateData)) + requireEventually.ElementsMatch(groups, getOrganizations(t, response.Status.Credential.ClientCertificateData)) + requireEventually.NotEmpty(response.Status.Credential.ClientKeyData) + requireEventually.NotNil(response.Status.Credential.ExpirationTimestamp) + requireEventually.InDelta(5*time.Minute, time.Until(response.Status.Credential.ExpirationTimestamp.Time), float64(time.Minute)) + }, 10*time.Second, 500*time.Millisecond) // Create a client using the certificate from the CredentialRequest. clientWithCertFromCredentialRequest := library.NewClientsetWithCertAndKey( diff --git a/test/integration/concierge_impersonation_proxy_test.go b/test/integration/concierge_impersonation_proxy_test.go index 717a52d1..031cb2e5 100644 --- a/test/integration/concierge_impersonation_proxy_test.go +++ b/test/integration/concierge_impersonation_proxy_test.go @@ -132,7 +132,6 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl mostRecentTokenCredentialRequestResponseLock.Lock() defer mostRecentTokenCredentialRequestResponseLock.Unlock() if mostRecentTokenCredentialRequestResponse == nil || credentialAlmostExpired(t, mostRecentTokenCredentialRequestResponse) { - var err error // Make a TokenCredentialRequest. This can either return a cert signed by the Kube API server's CA (e.g. on kind) // or a cert signed by the impersonator's signing CA (e.g. on GKE). Either should be accepted by the impersonation // proxy server as a valid authentication. @@ -140,23 +139,22 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl // However, we issue short-lived certs, so this cert will only be valid for a few minutes. // Cache it until it is almost expired and then refresh it whenever it is close to expired. // - require.Eventually(t, func() bool { - mostRecentTokenCredentialRequestResponse, err = createTokenCredentialRequest(credentialRequestSpecWithWorkingCredentials, client) - if err != nil { - t.Logf("failed to make TokenCredentialRequest: %s", library.Sdump(err)) - return false - } - return mostRecentTokenCredentialRequestResponse.Status.Credential != nil + library.RequireEventually(t, func(requireEventually *require.Assertions) { + resp, err := createTokenCredentialRequest(credentialRequestSpecWithWorkingCredentials, client) + requireEventually.NoError(err) + requireEventually.NotNil(resp) + requireEventually.NotNil(resp.Status) + requireEventually.NotNil(resp.Status.Credential) + requireEventually.Nilf(resp.Status.Message, "expected no error message but got: %s", library.Sdump(resp.Status.Message)) + requireEventually.NotEmpty(resp.Status.Credential.ClientCertificateData) + requireEventually.NotEmpty(resp.Status.Credential.ClientKeyData) + + // At the moment the credential request should not have returned a token. In the future, if we make it return + // tokens, we should revisit this test's rest config below. + requireEventually.Empty(resp.Status.Credential.Token) + + mostRecentTokenCredentialRequestResponse = resp }, 5*time.Minute, 5*time.Second) - - require.Nil(t, mostRecentTokenCredentialRequestResponse.Status.Message, - "expected no error message but got: %s", library.Sdump(mostRecentTokenCredentialRequestResponse.Status.Message)) - require.NotEmpty(t, mostRecentTokenCredentialRequestResponse.Status.Credential.ClientCertificateData) - require.NotEmpty(t, mostRecentTokenCredentialRequestResponse.Status.Credential.ClientKeyData) - - // At the moment the credential request should not have returned a token. In the future, if we make it return - // tokens, we should revisit this test's rest config below. - require.Empty(t, mostRecentTokenCredentialRequestResponse.Status.Credential.Token) } return mostRecentTokenCredentialRequestResponse.Status.Credential @@ -471,11 +469,13 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl // Make sure that all of the created ConfigMaps show up in the informer's cache to // demonstrate that the informer's "watch" verb is working through the impersonation proxy. - require.Eventually(t, func() bool { - _, err1 := informer.Lister().ConfigMaps(namespaceName).Get("configmap-1") - _, err2 := informer.Lister().ConfigMaps(namespaceName).Get("configmap-2") - _, err3 := informer.Lister().ConfigMaps(namespaceName).Get("configmap-3") - return err1 == nil && err2 == nil && err3 == nil + library.RequireEventually(t, func(requireEventually *require.Assertions) { + _, err := informer.Lister().ConfigMaps(namespaceName).Get("configmap-1") + requireEventually.NoError(err) + _, err = informer.Lister().ConfigMaps(namespaceName).Get("configmap-2") + requireEventually.NoError(err) + _, err = informer.Lister().ConfigMaps(namespaceName).Get("configmap-3") + requireEventually.NoError(err) }, 10*time.Second, 50*time.Millisecond) // Test "get" verb through the impersonation proxy. @@ -496,9 +496,10 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl require.Equal(t, "bar", updateResult.Data["foo"]) // Make sure that the updated ConfigMap shows up in the informer's cache. - require.Eventually(t, func() bool { + library.RequireEventually(t, func(requireEventually *require.Assertions) { configMap, err := informer.Lister().ConfigMaps(namespaceName).Get("configmap-3") - return err == nil && configMap.Data["foo"] == "bar" + requireEventually.NoError(err) + requireEventually.Equal("bar", configMap.Data["foo"]) }, 10*time.Second, 50*time.Millisecond) // Test "patch" verb through the impersonation proxy. @@ -513,9 +514,11 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl require.Equal(t, "42", patchResult.Data["baz"]) // Make sure that the patched ConfigMap shows up in the informer's cache. - require.Eventually(t, func() bool { + library.RequireEventually(t, func(requireEventually *require.Assertions) { configMap, err := informer.Lister().ConfigMaps(namespaceName).Get("configmap-3") - return err == nil && configMap.Data["foo"] == "bar" && configMap.Data["baz"] == "42" + requireEventually.NoError(err) + requireEventually.Equal("bar", configMap.Data["foo"]) + requireEventually.Equal("42", configMap.Data["baz"]) }, 10*time.Second, 50*time.Millisecond) // Test "delete" verb through the impersonation proxy. @@ -523,10 +526,13 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl require.NoError(t, err) // Make sure that the deleted ConfigMap shows up in the informer's cache. - require.Eventually(t, func() bool { - _, getErr := informer.Lister().ConfigMaps(namespaceName).Get("configmap-3") - list, listErr := informer.Lister().ConfigMaps(namespaceName).List(configMapLabels.AsSelector()) - return k8serrors.IsNotFound(getErr) && listErr == nil && len(list) == 2 + library.RequireEventually(t, func(requireEventually *require.Assertions) { + _, err := informer.Lister().ConfigMaps(namespaceName).Get("configmap-3") + requireEventually.Truef(k8serrors.IsNotFound(err), "expected a NotFound error from get, got %v", err) + + list, err := informer.Lister().ConfigMaps(namespaceName).List(configMapLabels.AsSelector()) + requireEventually.NoError(err) + requireEventually.Len(list, 2) }, 10*time.Second, 50*time.Millisecond) // Test "deletecollection" verb through the impersonation proxy. @@ -534,9 +540,10 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl require.NoError(t, err) // Make sure that the deleted ConfigMaps shows up in the informer's cache. - require.Eventually(t, func() bool { - list, listErr := informer.Lister().ConfigMaps(namespaceName).List(configMapLabels.AsSelector()) - return listErr == nil && len(list) == 0 + library.RequireEventually(t, func(requireEventually *require.Assertions) { + list, err := informer.Lister().ConfigMaps(namespaceName).List(configMapLabels.AsSelector()) + requireEventually.NoError(err) + requireEventually.Empty(list) }, 10*time.Second, 50*time.Millisecond) // There should be no ConfigMaps left. @@ -1033,7 +1040,16 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl // see that we can read stdout and it spits out stdin output back to us wantAttachStdout := fmt.Sprintf("VAR: %s\n", echoString) - require.Eventuallyf(t, func() bool { return attachStdout.String() == wantAttachStdout }, time.Second*60, time.Millisecond*250, `got "kubectl attach" stdout: %q, wanted: %q (stderr: %q)`, attachStdout.String(), wantAttachStdout, attachStderr.String()) + library.RequireEventually(t, func(requireEventually *require.Assertions) { + requireEventually.Equal( + wantAttachStdout, + attachStdout.String(), + `got "kubectl attach" stdout: %q, wanted: %q (stderr: %q)`, + attachStdout.String(), + wantAttachStdout, + attachStderr.String(), + ) + }, time.Second*60, time.Millisecond*250) // close stdin and attach process should exit err = attachStdin.Close() @@ -1560,20 +1576,20 @@ func TestImpersonationProxy(t *testing.T) { //nolint:gocyclo // yeah, it's compl // so we'll skip this check on clusters which have load balancers but don't run the squid proxy. // The other cluster types that do run the squid proxy will give us sufficient coverage here. if env.Proxy != "" { - require.Eventually(t, func() bool { + library.RequireEventually(t, func(requireEventually *require.Assertions) { // It's okay if this returns RBAC errors because this user has no role bindings. // What we want to see is that the proxy eventually shuts down entirely. _, err := impersonationProxyViaSquidKubeClientWithoutCredential(t, proxyServiceEndpoint).CoreV1().Namespaces().List(ctx, metav1.ListOptions{}) isErr, _ := isServiceUnavailableViaSquidError(err, proxyServiceEndpoint) - return isErr + requireEventually.Truef(isErr, "wanted service unavailable via squid error, got %v", err) }, 20*time.Second, 500*time.Millisecond) } // Check that the generated TLS cert Secret was deleted by the controller because it's supposed to clean this up // when we disable the impersonator. - require.Eventually(t, func() bool { + library.RequireEventually(t, func(requireEventually *require.Assertions) { _, err := adminClient.CoreV1().Secrets(env.ConciergeNamespace).Get(ctx, impersonationProxyTLSSecretName(env), metav1.GetOptions{}) - return k8serrors.IsNotFound(err) + requireEventually.Truef(k8serrors.IsNotFound(err), "expected NotFound error, got %v", err) }, 10*time.Second, 250*time.Millisecond) // Check that the generated CA cert Secret was not deleted by the controller because it's supposed to keep this diff --git a/test/integration/ldap_client_test.go b/test/integration/ldap_client_test.go index e972fc49..b12d0c67 100644 --- a/test/integration/ldap_client_test.go +++ b/test/integration/ldap_client_test.go @@ -775,28 +775,11 @@ func startLongRunningCommandAndWaitForInitialOutput( require.NoError(t, err) }) - earlyTerminationCh := make(chan bool, 1) - go func() { - err = cmd.Wait() - earlyTerminationCh <- true - }() - - terminatedEarly := false - require.Eventually(t, func() bool { + library.RequireEventually(t, func(requireEventually *require.Assertions) { t.Logf(`Waiting for %s to emit output: "%s"`, command, waitForOutputToContain) - if strings.Contains(watchOn.String(), waitForOutputToContain) { - return true - } - select { - case <-earlyTerminationCh: - terminatedEarly = true - return true - default: // ignore when this non-blocking read found no message - } - return false + requireEventually.Equal(-1, cmd.ProcessState.ExitCode(), "subcommand ended sooner than expected") + requireEventually.Contains(watchOn.String(), waitForOutputToContain, "expected process to emit output") }, 1*time.Minute, 1*time.Second) - require.Falsef(t, terminatedEarly, "subcommand ended sooner than expected") - t.Logf("Detected that %s has started successfully", command) } diff --git a/test/integration/supervisor_discovery_test.go b/test/integration/supervisor_discovery_test.go index 6740d644..c110d750 100644 --- a/test/integration/supervisor_discovery_test.go +++ b/test/integration/supervisor_discovery_test.go @@ -17,7 +17,6 @@ import ( "testing" "time" - "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" k8serrors "k8s.io/apimachinery/pkg/api/errors" @@ -398,15 +397,12 @@ func requireEndpointNotFound(t *testing.T, url, host, caBundle string) { requestNonExistentPath.Host = host - var response *http.Response - assert.Eventually(t, func() bool { - response, err = httpClient.Do(requestNonExistentPath) //nolint:bodyclose - return err == nil && response.StatusCode == http.StatusNotFound + library.RequireEventually(t, func(requireEventually *require.Assertions) { + response, err := httpClient.Do(requestNonExistentPath) + requireEventually.NoError(err) + requireEventually.NoError(response.Body.Close()) + requireEventually.Equal(http.StatusNotFound, response.StatusCode) }, time.Minute, 200*time.Millisecond) - require.NoError(t, err) - require.Equal(t, http.StatusNotFound, response.StatusCode) - err = response.Body.Close() - require.NoError(t, err) } func requireEndpointHasTLSErrorBecauseCertificatesAreNotReady(t *testing.T, url string) { @@ -415,15 +411,17 @@ func requireEndpointHasTLSErrorBecauseCertificatesAreNotReady(t *testing.T, url ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) defer cancel() - request, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil) - require.NoError(t, err) + library.RequireEventually(t, func(requireEventually *require.Assertions) { + request, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil) + requireEventually.NoError(err) - assert.Eventually(t, func() bool { - _, err = httpClient.Do(request) //nolint:bodyclose - return err != nil && strings.Contains(err.Error(), "tls: unrecognized name") + response, err := httpClient.Do(request) + if err == nil { + _ = response.Body.Close() + } + requireEventually.Error(err) + requireEventually.EqualError(err, fmt.Sprintf(`Get "%s": remote error: tls: unrecognized name`, url)) }, time.Minute, 200*time.Millisecond) - require.Error(t, err) - require.EqualError(t, err, fmt.Sprintf(`Get "%s": remote error: tls: unrecognized name`, url)) } func requireCreatingFederationDomainCausesDiscoveryEndpointsToAppear( @@ -553,17 +551,19 @@ func requireSuccessEndpointResponse(t *testing.T, endpointURL, issuer, caBundle // Fetch that discovery endpoint. Give it some time for the endpoint to come into existence. var response *http.Response - assert.Eventually(t, func() bool { - response, err = httpClient.Do(requestDiscoveryEndpoint) //nolint:bodyclose - return err == nil && response.StatusCode == http.StatusOK - }, time.Minute, 200*time.Millisecond) - require.NoError(t, err) - require.Equal(t, http.StatusOK, response.StatusCode) + var responseBody []byte + library.RequireEventually(t, func(requireEventually *require.Assertions) { + var err error + response, err = httpClient.Do(requestDiscoveryEndpoint) + requireEventually.NoError(err) + defer func() { _ = response.Body.Close() }() + + requireEventually.Equal(http.StatusOK, response.StatusCode) + + responseBody, err = ioutil.ReadAll(response.Body) + requireEventually.NoError(err) + }, time.Minute, 200*time.Millisecond) - responseBody, err := ioutil.ReadAll(response.Body) - require.NoError(t, err) - err = response.Body.Close() - require.NoError(t, err) return response, string(responseBody) } @@ -603,26 +603,16 @@ func requireDelete(t *testing.T, client pinnipedclientset.Interface, ns, name st func requireStatus(t *testing.T, client pinnipedclientset.Interface, ns, name string, status v1alpha1.FederationDomainStatusCondition) { t.Helper() - var federationDomain *v1alpha1.FederationDomain - var err error - assert.Eventually(t, func() bool { + library.RequireEventually(t, func(requireEventually *require.Assertions) { ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) defer cancel() - federationDomain, err = client.ConfigV1alpha1().FederationDomains(ns).Get(ctx, name, metav1.GetOptions{}) - if err != nil { - t.Logf("error trying to get FederationDomain %s/%s: %v", ns, name, err) - return false - } + federationDomain, err := client.ConfigV1alpha1().FederationDomains(ns).Get(ctx, name, metav1.GetOptions{}) + requireEventually.NoError(err) - if federationDomain.Status.Status != status { - t.Logf("found FederationDomain %s/%s with status %s", ns, name, federationDomain.Status.Status) - return false - } - return true + t.Logf("found FederationDomain %s/%s with status %s", ns, name, federationDomain.Status.Status) + requireEventually.Equalf(status, federationDomain.Status.Status, "unexpected status (message = '%s')", federationDomain.Status.Message) }, 5*time.Minute, 200*time.Millisecond) - require.NoError(t, err) - require.Equalf(t, status, federationDomain.Status.Status, "unexpected status (message = '%s')", federationDomain.Status.Message) } func newHTTPClient(t *testing.T, caBundle string, dnsOverrides map[string]string) *http.Client { diff --git a/test/integration/supervisor_login_test.go b/test/integration/supervisor_login_test.go index 7d811da7..b53c211d 100644 --- a/test/integration/supervisor_login_test.go +++ b/test/integration/supervisor_login_test.go @@ -354,26 +354,23 @@ func testSupervisorLogin( nil, ) require.NoError(t, err) - var jwksRequestStatus int - assert.Eventually(t, func() bool { + library.RequireEventually(t, func(requireEventually *require.Assertions) { rsp, err := httpClient.Do(requestJWKSEndpoint) - require.NoError(t, err) - require.NoError(t, rsp.Body.Close()) - jwksRequestStatus = rsp.StatusCode - return jwksRequestStatus == http.StatusOK + requireEventually.NoError(err) + requireEventually.NoError(rsp.Body.Close()) + requireEventually.Equal(http.StatusOK, rsp.StatusCode) }, 30*time.Second, 200*time.Millisecond) - require.Equal(t, http.StatusOK, jwksRequestStatus) // Create upstream IDP and wait for it to become ready. createIDP(t) // Perform OIDC discovery for our downstream. var discovery *coreosoidc.Provider - assert.Eventually(t, func() bool { + library.RequireEventually(t, func(requireEventually *require.Assertions) { + var err error discovery, err = coreosoidc.NewProvider(oidcHTTPClientContext, downstream.Spec.Issuer) - return err == nil + requireEventually.NoError(err) }, 30*time.Second, 200*time.Millisecond) - require.NoError(t, err) // Start a callback server on localhost. localCallbackServer := startLocalCallbackServer(t) diff --git a/test/integration/supervisor_secrets_test.go b/test/integration/supervisor_secrets_test.go index 2cf44128..229c0ec4 100644 --- a/test/integration/supervisor_secrets_test.go +++ b/test/integration/supervisor_secrets_test.go @@ -9,7 +9,6 @@ import ( "testing" "time" - "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "gopkg.in/square/go-jose.v2" corev1 "k8s.io/api/core/v1" @@ -76,16 +75,15 @@ func TestSupervisorSecrets(t *testing.T) { t.Run(test.name, func(t *testing.T) { // Ensure a secret is created with the FederationDomain's JWKS. var updatedFederationDomain *configv1alpha1.FederationDomain - var err error - assert.Eventually(t, func() bool { - updatedFederationDomain, err = supervisorClient. + library.RequireEventually(t, func(requireEventually *require.Assertions) { + resp, err := supervisorClient. ConfigV1alpha1(). FederationDomains(env.SupervisorNamespace). Get(ctx, federationDomain.Name, metav1.GetOptions{}) - return err == nil && test.secretName(updatedFederationDomain) != "" + requireEventually.NoError(err) + requireEventually.NotEmpty(test.secretName(resp)) + updatedFederationDomain = resp }, time.Second*10, time.Millisecond*500) - require.NoError(t, err) - require.NotEmpty(t, test.secretName(updatedFederationDomain)) // Ensure the secret actually exists. secret, err := kubeClient. @@ -109,14 +107,14 @@ func TestSupervisorSecrets(t *testing.T) { Secrets(env.SupervisorNamespace). Delete(ctx, test.secretName(updatedFederationDomain), metav1.DeleteOptions{}) require.NoError(t, err) - assert.Eventually(t, func() bool { + library.RequireEventually(t, func(requireEventually *require.Assertions) { + var err error secret, err = kubeClient. CoreV1(). Secrets(env.SupervisorNamespace). Get(ctx, test.secretName(updatedFederationDomain), metav1.GetOptions{}) - return err == nil + requireEventually.NoError(err) }, time.Second*10, time.Millisecond*500) - require.NoError(t, err) // Ensure that the new secret is valid. test.ensureValid(t, secret) diff --git a/test/integration/supervisor_storage_garbage_collection_test.go b/test/integration/supervisor_storage_garbage_collection_test.go index 1394b6f4..219a0537 100644 --- a/test/integration/supervisor_storage_garbage_collection_test.go +++ b/test/integration/supervisor_storage_garbage_collection_test.go @@ -9,7 +9,6 @@ import ( "testing" "time" - "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" v1 "k8s.io/api/core/v1" k8serrors "k8s.io/apimachinery/pkg/api/errors" @@ -35,14 +34,6 @@ func TestStorageGarbageCollection(t *testing.T) { secretWhichWillExpireBeforeTheTestEnds := createSecret(ctx, t, secrets, "near-future", time.Now().Add(30*time.Second)) secretNotYetExpired := createSecret(ctx, t, secrets, "far-future", time.Now().Add(10*time.Minute)) - var err error - secretIsNotFound := func(secretName string) func() bool { - return func() bool { - _, err = secrets.Get(ctx, secretName, metav1.GetOptions{}) - return k8serrors.IsNotFound(err) - } - } - // Start a background goroutine which will end as soon as the test ends. // Keep updating a secret which has the "storage.pinniped.dev/garbage-collect-after" annotation // in the same namespace just to get the controller to respond faster. @@ -64,13 +55,18 @@ func TestStorageGarbageCollection(t *testing.T) { // in practice we should only need to wait about 30 seconds, which is the GC controller's self-imposed // rate throttling time period. slightlyLongerThanGCControllerFullResyncPeriod := 3*time.Minute + 30*time.Second - assert.Eventually(t, secretIsNotFound(secretAlreadyExpired.Name), slightlyLongerThanGCControllerFullResyncPeriod, 250*time.Millisecond) - require.Truef(t, k8serrors.IsNotFound(err), "wanted a NotFound error but got %v", err) // prints out the error and stops the test in case of failure - assert.Eventually(t, secretIsNotFound(secretWhichWillExpireBeforeTheTestEnds.Name), slightlyLongerThanGCControllerFullResyncPeriod, 250*time.Millisecond) - require.Truef(t, k8serrors.IsNotFound(err), "wanted a NotFound error but got %v", err) // prints out the error and stops the test in case of failure + library.RequireEventually(t, func(requireEventually *require.Assertions) { + _, err := secrets.Get(ctx, secretAlreadyExpired.Name, metav1.GetOptions{}) + requireEventually.Truef(k8serrors.IsNotFound(err), "wanted a NotFound error but got %v", err) + }, slightlyLongerThanGCControllerFullResyncPeriod, 250*time.Millisecond) + + library.RequireEventually(t, func(requireEventually *require.Assertions) { + _, err := secrets.Get(ctx, secretWhichWillExpireBeforeTheTestEnds.Name, metav1.GetOptions{}) + requireEventually.Truef(k8serrors.IsNotFound(err), "wanted a NotFound error but got %v", err) + }, slightlyLongerThanGCControllerFullResyncPeriod, 250*time.Millisecond) // The unexpired secret should not have been deleted within the timeframe of this test run. - _, err = secrets.Get(ctx, secretNotYetExpired.Name, metav1.GetOptions{}) + _, err := secrets.Get(ctx, secretNotYetExpired.Name, metav1.GetOptions{}) require.NoError(t, err) } diff --git a/test/library/access.go b/test/library/access.go index 46c31bd3..f2cae320 100644 --- a/test/library/access.go +++ b/test/library/access.go @@ -11,10 +11,8 @@ import ( "testing" "time" - "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" authorizationv1 "k8s.io/api/authorization/v1" - v1 "k8s.io/api/core/v1" rbacv1 "k8s.io/api/rbac/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/kubernetes" @@ -39,16 +37,12 @@ func AccessAsUserTest( addTestClusterUserCanViewEverythingRoleBinding(t, testUsername) // Use the client which is authenticated as the test user to list namespaces - var listNamespaceResponse *v1.NamespaceList - var err error - var canListNamespaces = func() bool { - listNamespaceResponse, err = clientUnderTest.CoreV1().Namespaces().List(ctx, metav1.ListOptions{}) - return err == nil - } - assert.Eventually(t, canListNamespaces, accessRetryTimeout, accessRetryInterval) - require.NoError(t, err) // prints out the error and stops the test in case of failure - require.NotNil(t, listNamespaceResponse) - require.NotEmpty(t, listNamespaceResponse.Items) + RequireEventually(t, func(requireEventually *require.Assertions) { + resp, err := clientUnderTest.CoreV1().Namespaces().List(ctx, metav1.ListOptions{}) + requireEventually.NoError(err) + requireEventually.NotNil(resp) + requireEventually.NotEmpty(resp.Items) + }, accessRetryTimeout, accessRetryInterval, "user never had access to list namespaces") } } @@ -61,16 +55,11 @@ func AccessAsUserWithKubectlTest( addTestClusterUserCanViewEverythingRoleBinding(t, testUsername) // Use the given kubeconfig with kubectl to list namespaces as the test user - var kubectlCommandOutput string - var err error - var canListNamespaces = func() bool { - kubectlCommandOutput, err = runKubectlGetNamespaces(t, testKubeConfigYAML) - return err == nil - } - - assert.Eventually(t, canListNamespaces, accessRetryTimeout, accessRetryInterval) - require.NoError(t, err) // prints out the error and stops the test in case of failure - require.Containsf(t, kubectlCommandOutput, expectedNamespace, "actual output: %q", kubectlCommandOutput) + RequireEventually(t, func(requireEventually *require.Assertions) { + kubectlCommandOutput, err := runKubectlGetNamespaces(t, testKubeConfigYAML) + requireEventually.NoError(err) + requireEventually.Containsf(kubectlCommandOutput, expectedNamespace, "actual output: %q", kubectlCommandOutput) + }, accessRetryTimeout, accessRetryInterval, "user never had access to list namespaces via kubectl") } } @@ -88,16 +77,12 @@ func AccessAsGroupTest( addTestClusterGroupCanViewEverythingRoleBinding(t, testGroup) // Use the client which is authenticated as the test user to list namespaces - var listNamespaceResponse *v1.NamespaceList - var err error - var canListNamespaces = func() bool { - listNamespaceResponse, err = clientUnderTest.CoreV1().Namespaces().List(ctx, metav1.ListOptions{}) - return err == nil - } - assert.Eventually(t, canListNamespaces, accessRetryTimeout, accessRetryInterval) - require.NoError(t, err) // prints out the error and stops the test in case of failure - require.NotNil(t, listNamespaceResponse) - require.NotEmpty(t, listNamespaceResponse.Items) + RequireEventually(t, func(requireEventually *require.Assertions) { + listNamespaceResponse, err := clientUnderTest.CoreV1().Namespaces().List(ctx, metav1.ListOptions{}) + requireEventually.NoError(err) + requireEventually.NotNil(listNamespaceResponse) + requireEventually.NotEmpty(listNamespaceResponse.Items) + }, accessRetryTimeout, accessRetryInterval, "user never had access to list namespaces") } } @@ -110,16 +95,11 @@ func AccessAsGroupWithKubectlTest( addTestClusterGroupCanViewEverythingRoleBinding(t, testGroup) // Use the given kubeconfig with kubectl to list namespaces as the test user - var kubectlCommandOutput string - var err error - var canListNamespaces = func() bool { - kubectlCommandOutput, err = runKubectlGetNamespaces(t, testKubeConfigYAML) - return err == nil - } - - assert.Eventually(t, canListNamespaces, accessRetryTimeout, accessRetryInterval) - require.NoError(t, err) // prints out the error and stops the test in case of failure - require.Containsf(t, kubectlCommandOutput, expectedNamespace, "actual output: %q", kubectlCommandOutput) + RequireEventually(t, func(requireEventually *require.Assertions) { + kubectlCommandOutput, err := runKubectlGetNamespaces(t, testKubeConfigYAML) + requireEventually.NoError(err) + requireEventually.Containsf(kubectlCommandOutput, expectedNamespace, "actual output: %q", kubectlCommandOutput) + }, accessRetryTimeout, accessRetryInterval, "user never had access to list namespaces") } } diff --git a/test/library/assertions.go b/test/library/assertions.go index 5fccbca3..1decb3ad 100644 --- a/test/library/assertions.go +++ b/test/library/assertions.go @@ -15,8 +15,112 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/wait" "k8s.io/client-go/kubernetes" + + "go.pinniped.dev/internal/constable" ) +type ( + // loopTestingT records the failures observed during an iteration of the RequireEventually() loop. + loopTestingT []assertionFailure + + // assertionFailure is a single error observed during an iteration of the RequireEventually() loop. + assertionFailure struct { + format string + args []interface{} + } +) + +// loopTestingT implements require.TestingT: +var _ require.TestingT = (*loopTestingT)(nil) + +// Errorf is called by the assert.Assertions methods to record an error. +func (e *loopTestingT) Errorf(format string, args ...interface{}) { + *e = append(*e, assertionFailure{format, args}) +} + +const errLoopFailNow = constable.Error("failing test now") + +// FailNow is called by the require.Assertions methods to force the code to immediately halt. It panics with a +// sentinel value that is recovered by recoverLoopFailNow(). +func (e *loopTestingT) FailNow() { panic(errLoopFailNow) } + +// ignoreFailNowPanic catches the panic from FailNow() and ignores it (allowing the FailNow() call to halt the test +// but let the retry loop continue. +func recoverLoopFailNow() { + switch p := recover(); p { + case nil, errLoopFailNow: + // Ignore nil (success) and our sentinel value. + return + default: + // Re-panic on any other value. + panic(p) + } +} + +func RequireEventuallyf( + t *testing.T, + f func(requireEventually *require.Assertions), + waitFor time.Duration, + tick time.Duration, + msg string, + args ...interface{}, +) { + RequireEventually(t, f, waitFor, tick, fmt.Sprintf(msg, args...)) +} + +// RequireEventually is similar to require.Eventually() except that it is thread safe and provides a richer way to +// write per-iteration assertions. +func RequireEventually( + t *testing.T, + f func(requireEventually *require.Assertions), + waitFor time.Duration, + tick time.Duration, + msgAndArgs ...interface{}, +) { + t.Helper() + + // Set up some bookkeeping so we can fail with a nice message if necessary. + var ( + startTime = time.Now() + attempts int + mostRecentFailures loopTestingT + ) + + // Run the check until it completes with no assertion failures. + waitErr := wait.PollImmediate(tick, waitFor, func() (bool, error) { + t.Helper() + attempts++ + + // Reset the recorded failures on each iteration. + mostRecentFailures = nil + + // Ignore any panics caused by FailNow() -- they will cause the f() to return immediately but any errors + // they've logged should be in mostRecentFailures. + defer recoverLoopFailNow() + + // Run the per-iteration check, recording any failed assertions into mostRecentFailures. + f(require.New(&mostRecentFailures)) + + // We're only done iterating if no assertions have failed. + return len(mostRecentFailures) == 0, nil + }) + + // If things eventually completed with no failures/timeouts, we're done. + if waitErr == nil { + return + } + + // Re-assert the most recent set of failures with a nice error log. + duration := time.Since(startTime).Round(100 * time.Millisecond) + t.Errorf("failed to complete even after %s (%d attempts): %v", duration, attempts, waitErr) + for _, failure := range mostRecentFailures { + t.Errorf(failure.format, failure.args...) + } + + // Fail the test now with the provided message. + require.NoError(t, waitErr, msgAndArgs...) +} + // RequireEventuallyWithoutError is similar to require.Eventually() except that it also allows the caller to // return an error from the condition function. If the condition function returns an error at any // point, the assertion will immediately fail. diff --git a/test/library/browsertest/browsertest.go b/test/library/browsertest/browsertest.go index cbfc5113..e84c3a5f 100644 --- a/test/library/browsertest/browsertest.go +++ b/test/library/browsertest/browsertest.go @@ -59,15 +59,13 @@ func Open(t *testing.T) *agouti.Page { func WaitForVisibleElements(t *testing.T, page *agouti.Page, selectors ...string) { t.Helper() - require.Eventuallyf(t, - func() bool { + library.RequireEventuallyf(t, + func(requireEventually *require.Assertions) { for _, sel := range selectors { vis, err := page.First(sel).Visible() - if !(err == nil && vis) { - return false - } + requireEventually.NoError(err) + requireEventually.Truef(vis, "expected element %q to be visible", sel) } - return true }, operationTimeout, operationPollingInterval, @@ -80,17 +78,15 @@ func WaitForVisibleElements(t *testing.T, page *agouti.Page, selectors ...string // to occur and times out, failing the test, if it never does. func WaitForURL(t *testing.T, page *agouti.Page, pat *regexp.Regexp) { var lastURL string - require.Eventuallyf(t, - func() bool { + library.RequireEventuallyf(t, + func(requireEventually *require.Assertions) { url, err := page.URL() - if err == nil && pat.MatchString(url) { - return true - } if url != lastURL { t.Logf("saw URL %s", url) lastURL = url } - return false + requireEventually.NoError(err) + requireEventually.Regexp(pat, url) }, operationTimeout, operationPollingInterval, diff --git a/test/library/client.go b/test/library/client.go index c9e9baca..6ee3bd30 100644 --- a/test/library/client.go +++ b/test/library/client.go @@ -15,7 +15,6 @@ import ( "testing" "time" - "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" authorizationv1 "k8s.io/api/authorization/v1" corev1 "k8s.io/api/core/v1" @@ -295,30 +294,20 @@ func CreateTestFederationDomain(ctx context.Context, t *testing.T, issuer string // Wait for the FederationDomain to enter the expected phase (or time out). var result *configv1alpha1.FederationDomain - assert.Eventuallyf(t, func() bool { + RequireEventuallyf(t, func(requireEventually *require.Assertions) { var err error result, err = federationDomains.Get(ctx, federationDomain.Name, metav1.GetOptions{}) - require.NoError(t, err) - return result.Status.Status == expectStatus - }, 60*time.Second, 1*time.Second, "expected the FederationDomain to have status %q", expectStatus) - require.Equal(t, expectStatus, result.Status.Status) + requireEventually.NoError(err) + requireEventually.Equal(expectStatus, result.Status.Status) - // If the FederationDomain was successfully created, ensure all secrets are present before continuing - if result.Status.Status == configv1alpha1.SuccessFederationDomainStatusCondition { - assert.Eventually(t, func() bool { - var err error - result, err = federationDomains.Get(ctx, federationDomain.Name, metav1.GetOptions{}) - require.NoError(t, err) - return result.Status.Secrets.JWKS.Name != "" && - result.Status.Secrets.TokenSigningKey.Name != "" && - result.Status.Secrets.StateSigningKey.Name != "" && - result.Status.Secrets.StateEncryptionKey.Name != "" - }, 60*time.Second, 1*time.Second, "expected the FederationDomain to have secrets populated") - require.NotEmpty(t, result.Status.Secrets.JWKS.Name) - require.NotEmpty(t, result.Status.Secrets.TokenSigningKey.Name) - require.NotEmpty(t, result.Status.Secrets.StateSigningKey.Name) - require.NotEmpty(t, result.Status.Secrets.StateEncryptionKey.Name) - } + // If the FederationDomain was successfully created, ensure all secrets are present before continuing + if expectStatus == configv1alpha1.SuccessFederationDomainStatusCondition { + requireEventually.NotEmpty(result.Status.Secrets.JWKS.Name, "expected status.secrets.jwks.name not to be empty") + requireEventually.NotEmpty(result.Status.Secrets.TokenSigningKey.Name, "expected status.secrets.tokenSigningKey.name not to be empty") + requireEventually.NotEmpty(result.Status.Secrets.StateSigningKey.Name, "expected status.secrets.stateSigningKey.name not to be empty") + requireEventually.NotEmpty(result.Status.Secrets.StateEncryptionKey.Name, "expected status.secrets.stateEncryptionKey.name not to be empty") + } + }, 60*time.Second, 1*time.Second, "expected the FederationDomain to have status %q", expectStatus) return federationDomain } @@ -391,14 +380,11 @@ func CreateTestOIDCIdentityProvider(t *testing.T, spec idpv1alpha1.OIDCIdentityP // Wait for the OIDCIdentityProvider to enter the expected phase (or time out). var result *idpv1alpha1.OIDCIdentityProvider - require.Eventuallyf(t, func() bool { + RequireEventuallyf(t, func(requireEventually *require.Assertions) { var err error result, err = upstreams.Get(ctx, created.Name, metav1.GetOptions{}) - if err != nil { - t.Logf("error while getting OIDCIdentityProvider %s/%s: %s", created.Namespace, created.Name, err.Error()) - return false - } - return result.Status.Phase == expectedPhase + requireEventually.NoErrorf(err, "error while getting OIDCIdentityProvider %s/%s", created.Namespace, created.Name) + requireEventually.Equal(expectedPhase, result.Status.Phase) }, 60*time.Second, 1*time.Second, "expected the OIDCIdentityProvider to go into phase %s, OIDCIdentityProvider was: %s", expectedPhase, Sdump(result)) return result } @@ -429,18 +415,18 @@ func CreateTestLDAPIdentityProvider(t *testing.T, spec idpv1alpha1.LDAPIdentityP // Wait for the LDAPIdentityProvider to enter the expected phase (or time out). var result *idpv1alpha1.LDAPIdentityProvider - require.Eventuallyf(t, func() bool { - var err error - result, err = upstreams.Get(ctx, created.Name, metav1.GetOptions{}) - if err != nil { - t.Logf("error while getting LDAPIdentityProvider %s/%s: %s", created.Namespace, created.Name, err.Error()) - return false - } - return result.Status.Phase == expectedPhase - }, + RequireEventuallyf(t, + func(requireEventually *require.Assertions) { + var err error + result, err = upstreams.Get(ctx, created.Name, metav1.GetOptions{}) + requireEventually.NoErrorf(err, "error while getting LDAPIdentityProvider %s/%s", created.Namespace, created.Name) + requireEventually.Equalf(expectedPhase, result.Status.Phase, "LDAPIdentityProvider is not in phase %s: %v", expectedPhase, Sdump(result)) + }, 2*time.Minute, // it takes 1 minute for a failed LDAP TLS connection test to timeout before it tries using StartTLS, so wait longer than that 1*time.Second, - "expected the LDAPIdentityProvider to go into phase %s, LDAPIdentityProvider was: %s", expectedPhase, Sdump(result)) + "expected the LDAPIdentityProvider to go into phase %s", + expectedPhase, + ) return result } @@ -502,11 +488,11 @@ func CreatePod(ctx context.Context, t *testing.T, name, namespace string, spec c }) var result *corev1.Pod - require.Eventuallyf(t, func() bool { + RequireEventuallyf(t, func(requireEventually *require.Assertions) { var err error result, err = pods.Get(ctx, created.Name, metav1.GetOptions{}) - require.NoError(t, err) - return result.Status.Phase == corev1.PodRunning + requireEventually.NoError(err) + requireEventually.Equal(corev1.PodRunning, result.Status.Phase) }, 15*time.Second, 1*time.Second, "expected the Pod to go into phase %s", corev1.PodRunning) return result }