From 9baffb9a173fb9755a7fd9c9380ef8bf9610a713 Mon Sep 17 00:00:00 2001 From: Daniel Garnier-Moiroux Date: Fri, 20 Dec 2024 18:21:27 +0100 Subject: [PATCH] tests: address flakyness in ScimUserEndpointsAliasMockMvcTests There were at least two issues: 1. There was a race condition between tests, because they all created a user with the same username, and some tests might find multiple users for a given username, when they were created too close in time. Since the PollutionPreventionExtension only runs beforeAll/afterAll tests, multiple tests inthe same class could find a duplicate user. 2. There is also an issue when creating a user using the HTTP api, and then querying with a sort-by. The GET /Users call might not return the newly created user immediately. By changing the query from using a sort-by to using a filter="userName eq ...", this seems to fix the broken tests. I ran 100 iterations of the flakey tests with this new query, and they did not fail on my machine. --- .../ScimUserEndpointsAliasMockMvcTests.java | 51 +++++++++++-------- 1 file changed, 30 insertions(+), 21 deletions(-) diff --git a/uaa/src/test/java/org/cloudfoundry/identity/uaa/scim/endpoints/ScimUserEndpointsAliasMockMvcTests.java b/uaa/src/test/java/org/cloudfoundry/identity/uaa/scim/endpoints/ScimUserEndpointsAliasMockMvcTests.java index d9585f5b2c4..e041560bf29 100644 --- a/uaa/src/test/java/org/cloudfoundry/identity/uaa/scim/endpoints/ScimUserEndpointsAliasMockMvcTests.java +++ b/uaa/src/test/java/org/cloudfoundry/identity/uaa/scim/endpoints/ScimUserEndpointsAliasMockMvcTests.java @@ -1,7 +1,6 @@ package org.cloudfoundry.identity.uaa.scim.endpoints; import com.fasterxml.jackson.core.type.TypeReference; - import org.cloudfoundry.identity.uaa.DefaultTestContext; import org.cloudfoundry.identity.uaa.alias.AliasMockMvcTestBase; import org.cloudfoundry.identity.uaa.mock.util.MockMvcUtils; @@ -14,6 +13,7 @@ import org.cloudfoundry.identity.uaa.scim.ScimUserAliasHandler; import org.cloudfoundry.identity.uaa.scim.jdbc.JdbcScimUserProvisioning; import org.cloudfoundry.identity.uaa.scim.services.ScimUserService; +import org.cloudfoundry.identity.uaa.util.AlphanumericRandomValueStringGenerator; import org.cloudfoundry.identity.uaa.util.JsonUtils; import org.cloudfoundry.identity.uaa.util.UaaStringUtils; import org.cloudfoundry.identity.uaa.zone.IdentityZone; @@ -26,8 +26,10 @@ import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.EnumSource; +import org.springframework.beans.factory.annotation.Autowired; import org.springframework.http.HttpMethod; import org.springframework.http.HttpStatus; +import org.springframework.jdbc.core.JdbcTemplate; import org.springframework.mock.web.MockHttpServletResponse; import org.springframework.test.util.ReflectionTestUtils; import org.springframework.test.web.servlet.MvcResult; @@ -52,14 +54,23 @@ @DefaultTestContext public class ScimUserEndpointsAliasMockMvcTests extends AliasMockMvcTestBase { + + // There is a race conditions where multiple users with the same name are created between tests + // We ensure that the tests are independent by having a custom username per test, that we clean + // up after every test. + private String TEST_USERNAME; + private IdentityProviderAliasHandler idpEntityAliasHandler; private IdentityProviderEndpoints identityProviderEndpoints; private ScimUserAliasHandler scimUserAliasHandler; private ScimUserEndpoints scimUserEndpoints; private ScimUserService scimUserService; + @Autowired + JdbcTemplate jdbcTemplate; @BeforeEach void setUp() throws Exception { + TEST_USERNAME = new AlphanumericRandomValueStringGenerator(12).generate().toLowerCase(); setUpTokensAndCustomZone(); idpEntityAliasHandler = requireNonNull(webApplicationContext.getBean(IdentityProviderAliasHandler.class)); @@ -69,6 +80,13 @@ void setUp() throws Exception { scimUserService = requireNonNull(webApplicationContext.getBean(ScimUserService.class)); } + @AfterEach + void tearDown() { + // Delete any test user that was created. Not strictly necessary, but it makes + // it easier to debug issues by having less users in the database. + jdbcTemplate.execute("DELETE FROM users WHERE username = '" + TEST_USERNAME + "'"); + } + @Nested class Read { @Nested @@ -117,10 +135,7 @@ private void shouldStillReturnAliasPropertiesOfUsersWithAliasCreatedBeforehand( assertThat(createdUserWithAlias.getAliasZid()).isNotBlank().isEqualTo(zone2.getId()); // read all users in zone 1 and search for created user - final List allUsersInZone1 = readRecentlyCreatedUsersInZone(zone1); - final Optional createdUserOpt = allUsersInZone1.stream() - .filter(user -> user.getUserName().equals(createdUserWithAlias.getUserName())) - .findFirst(); + final Optional createdUserOpt = readUserFromZoneByUsername(scimUser.getUserName(), zone1); assertThat(createdUserOpt).isPresent(); // check if the user has non-empty alias properties @@ -242,13 +257,7 @@ private void shouldAccept_ShouldCreateAliasUser( final ScimUser createdScimUser = createScimUser(zone1, scimUser); // find alias user - //TODO this is flaky - final List usersZone2 = readRecentlyCreatedUsersInZone(zone2); - final Optional aliasUserOpt = usersZone2.stream() - .filter(user -> user.getId().equals(createdScimUser.getAliasId())) - .findFirst(); - assertThat(aliasUserOpt).isPresent(); - + final Optional aliasUserOpt = readUserFromZoneByUsername(scimUser.getUserName(), zone2); assertIsCorrectAliasPair(createdScimUser, aliasUserOpt.get(), zone2); } @@ -1588,7 +1597,7 @@ private static void assertIsCorrectAliasPair( assertThat(originalUser.getSchemas()).isEqualTo(aliasUser.getSchemas()); } - private static ScimUser buildScimUser( + private ScimUser buildScimUser( final String origin, final String zoneId, final String aliasId, @@ -1600,9 +1609,9 @@ private static ScimUser buildScimUser( scimUser.setAliasZid(aliasZid); scimUser.setZoneId(zoneId); - scimUser.setUserName("john.doe"); + scimUser.setUserName(TEST_USERNAME); scimUser.setName(new ScimUser.Name("John", "Doe")); - scimUser.setPrimaryEmail("john.doe@example.com"); + scimUser.setPrimaryEmail(TEST_USERNAME + "@example.com"); scimUser.setPassword("some-password"); return scimUser; @@ -1651,21 +1660,21 @@ private ScimUser createIdpWithAliasAndUserWithoutAlias( return createScimUser(zone1, scimUser); } - private List readRecentlyCreatedUsersInZone(final IdentityZone zone) throws Exception { + private Optional readUserFromZoneByUsername(String username, IdentityZone zone) throws Exception { final MockHttpServletRequestBuilder getRequestBuilder = get("/Users") .header(IdentityZoneSwitchingFilter.SUBDOMAIN_HEADER, zone.getSubdomain()) .header("Authorization", "Bearer " + getAccessTokenForZone(zone.getId())) - // return most recent users in first page to avoid querying for further pages - .param("sortBy", "created") - .param("sortOrder", "descending"); - final MvcResult getResult = mockMvc.perform(getRequestBuilder).andExpect(status().isOk()).andReturn(); + .param("filter", "userName eq \"%s\"".formatted(username)); + final MvcResult getResult = mockMvc.perform(getRequestBuilder) + .andExpect(status().isOk()) + .andReturn(); final SearchResults searchResults = JsonUtils.readValue( getResult.getResponse().getContentAsString(), new TypeReference<>() { } ); assertThat(searchResults).isNotNull(); - return searchResults.getResources(); + return searchResults.getResources().stream().findFirst(); } private Optional readUserFromZoneIfExists(final String id, final String zoneId) throws Exception {