Further Integration Tests for Alias Identity Providers Feature #2722

adrianhoelzl-sap · 2024-02-09T08:46:45Z

This PR adds additional MockMvc tests for some yet uncovered paths as well as for the GET IdPs endpoint.

cf-gitbot · 2024-02-09T08:46:48Z

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/187017042

The labels on this github issue will be updated when the story is started.

bruce-ricard

It's hard to review as I don't know what code was moved (and git thinks it was deleted and rewritten), and which code is indeed new, and which one was deleted.

Do you think you could try to split it into multiple smaller commits?

bruce-ricard · 2024-02-26T16:05:22Z

...org/cloudfoundry/identity/uaa/mock/providers/IdentityProviderEndpointsAliasMockMvcTests.java

@@ -327,85 +373,358 @@ protected UpdateBase(final boolean aliasFeatureEnabled) {
            void setUp() {
                arrangeAliasFeatureEnabled(aliasFeatureEnabled);
            }
-        }
-
-        @Nested


This is confusing.

It looks like you didn't actually delete this, but that git is confused and believes so. It makes it harder to review. That's one of the many reasons I like smaller commits, it makes it easier on git and us.

For this particular PR, it might be easier to just consider the new version of the test class and ignore the diffs git generated. The PDF I posted in the Slack channel might be helpful as well.

But then I don't see what is being deleted. I can't validate that we are not removing test coverage.

The commit says that it adds further tests, but I see lots of lines of code being deleted.

Tallicia · 2024-03-18T20:48:25Z

As discussed in last week's sync up, @adrianhoelzl-sap when you have updated the commits for this, I'll review as @bruce-ricard is out this week.

cf-gitbot · 2024-03-19T12:32:25Z

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/187268528

The labels on this github issue will be updated when the story is started.

adrianhoelzl-sap · 2024-03-19T12:34:22Z

As discussed in last week's sync up, @adrianhoelzl-sap when you have updated the commits for this, I'll review as @bruce-ricard is out this week.

Thanks for the update. I'll convert this PR back from draft to regular once it is ready for review.

…pdate method

…ias and the aliasId is set in the update payload (valid for enabled and disabled alias feature)

…y aliasId

…e to alias in existing IdP if alias feature is disabled

…xists and the update payload contains a non-empty aliasZid

…iesChangedWhileAliasPropertiesUnchanged to nested class ExistingAlias

…PropertiesToNull to nested class ExistingAlias

…ertiesToNullAndChangeOtherProperties to nested class ExistingAlias

…AliasIdOfExistingIdpMissing to nested class ExistingAlias

…DanglingReference to nested class ExistingAlias

…etToNull to nested class ExistingAlias

…SetToNull to nested class ExistingAlias

Tallicia · 2024-03-27T16:08:25Z

...org/cloudfoundry/identity/uaa/mock/providers/IdentityProviderEndpointsAliasMockMvcTests.java

+                @Test
+                void shouldReject_AliasIdNotSetInPayload_CustomToUaaZone() throws Exception {
+                    shouldReject_AliasIdNotSetInPayload(customZone, IdentityZone.getUaa());
+                }


Looks good. Leaving comments to track my progress as this is challenging to read through.

Tallicia · 2024-03-27T16:08:27Z

...org/cloudfoundry/identity/uaa/mock/providers/IdentityProviderEndpointsAliasMockMvcTests.java

+                @Test
+                void shouldReject_AliasIdNotSetInPayload_UaaToCustomZone() throws Exception {
+                    shouldReject_AliasIdNotSetInPayload(IdentityZone.getUaa(), customZone);
+                }


Looks good. Leaving comments to track my progress as this is challenging to read through.

Tallicia · 2024-03-27T16:08:42Z

...org/cloudfoundry/identity/uaa/mock/providers/IdentityProviderEndpointsAliasMockMvcTests.java

+                @Test
+                void shouldAccept_OtherPropertiesOfIdpWithAliasAreChanged_CustomToUaaZone() throws Exception {
+                    shouldAccept_OtherPropertiesOfIdpWithAliasAreChanged(customZone, IdentityZone.getUaa());
+                }


Looks good. Leaving comments to track my progress as this is challenging to read through.

Tallicia · 2024-03-27T16:08:51Z

...org/cloudfoundry/identity/uaa/mock/providers/IdentityProviderEndpointsAliasMockMvcTests.java

+            class ExistingAlias {
+                @Test
+                void shouldAccept_OtherPropertiesOfIdpWithAliasAreChanged_UaaToCustomZone() throws Exception {
+                    shouldAccept_OtherPropertiesOfIdpWithAliasAreChanged(IdentityZone.getUaa(), customZone);


Looks good. Leaving comments to track my progress as this is challenging to read through.

Tallicia · 2024-03-27T16:09:30Z

...org/cloudfoundry/identity/uaa/mock/providers/IdentityProviderEndpointsAliasMockMvcTests.java

+                    );
+                    idp.setAliasZid(zone.getId());
+                    shouldRejectUpdate(zone, idp, HttpStatus.UNPROCESSABLE_ENTITY);
+                }


Looks good. Leaving comments to track my progress as this is challenging to read through.

Tallicia · 2024-03-27T16:09:34Z

...org/cloudfoundry/identity/uaa/mock/providers/IdentityProviderEndpointsAliasMockMvcTests.java

+                @Test
+                void shouldReject_AliasZidSetToSameZone_CustomZone() throws Exception {
+                    shouldReject_AliasZidSetToSameZone(customZone);
+                }


Looks good. Leaving comments to track my progress as this is challenging to read through.

Tallicia · 2024-03-27T16:09:39Z

...org/cloudfoundry/identity/uaa/mock/providers/IdentityProviderEndpointsAliasMockMvcTests.java

+                @Test
+                void shouldReject_AliasZidSetToSameZone_UaaZone() throws Exception {
+                    shouldReject_AliasZidSetToSameZone(IdentityZone.getUaa());
+                }


Looks good. Leaving comments to track my progress as this is challenging to read through.

Tallicia · 2024-03-27T16:11:59Z

...org/cloudfoundry/identity/uaa/mock/providers/IdentityProviderEndpointsAliasMockMvcTests.java

+                    // try to create an alias for the IdP -> should fail because of the IdP's type
+                    createdProvider.setAliasZid(zone2.getId());
+                    shouldRejectUpdate(zone1, createdProvider, HttpStatus.UNPROCESSABLE_ENTITY);
+                }


Looks good. Leaving comments to track my progress as this is challenging to read through.

Tallicia · 2024-03-27T16:12:08Z

...org/cloudfoundry/identity/uaa/mock/providers/IdentityProviderEndpointsAliasMockMvcTests.java

+                private void shouldReject_AliasNotSupportedForIdpType(final IdentityZone zone1, final IdentityZone zone2) throws Exception {
+                    final IdentityProvider<?> uaaIdp = buildUaaIdpWithAliasProperties(zone1.getId(), null, null);
+                    final IdentityProvider<?> createdProvider = createIdp(zone1, uaaIdp);
+                    assertThat(createdProvider.getAliasZid()).isBlank();


Looks good. Leaving comments to track my progress as this is challenging to read through.

Tallicia · 2024-03-27T16:12:21Z

...org/cloudfoundry/identity/uaa/mock/providers/IdentityProviderEndpointsAliasMockMvcTests.java

+                @Test
+                void shouldReject_AliasNotSupportedForIdpType_CustomZone() throws Exception {
+                    shouldReject_AliasNotSupportedForIdpType(customZone, IdentityZone.getUaa());
+                }


Looks good. Leaving comments to track my progress as this is challenging to read through.

Tallicia · 2024-03-27T16:12:24Z

...org/cloudfoundry/identity/uaa/mock/providers/IdentityProviderEndpointsAliasMockMvcTests.java

+                @Test
+                void shouldReject_AliasNotSupportedForIdpType_UaaToCustomZone() throws Exception {
+                    shouldReject_AliasNotSupportedForIdpType(IdentityZone.getUaa(), customZone);
+                }


Looks good. Leaving comments to track my progress as this is challenging to read through.

Tallicia · 2024-03-27T16:13:59Z

...org/cloudfoundry/identity/uaa/mock/providers/IdentityProviderEndpointsAliasMockMvcTests.java

+
+                    // the alias IdP should have its reference removed
+                    assertReferenceWasRemovedFromAlias(initialAliasId, initialAliasZid);
+                }


Looks good. Leaving comments to track my progress as this is challenging to read through.

Tallicia · 2024-03-27T16:14:09Z

...org/cloudfoundry/identity/uaa/mock/providers/IdentityProviderEndpointsAliasMockMvcTests.java

+                @Test
+                void shouldAccept_SetOnlyAliasPropertiesToNull_UaaToCustomZone() throws Throwable {
+                    shouldAccept_SetOnlyAliasPropertiesToNull(IdentityZone.getUaa(), customZone);
+                }


Looks good. Leaving comments to track my progress as this is challenging to read through.

Tallicia · 2024-03-27T16:15:30Z

...org/cloudfoundry/identity/uaa/mock/providers/IdentityProviderEndpointsAliasMockMvcTests.java

+                existingIdp.setAliasId(null);
+                existingIdp.setAliasZid(null);
+                existingIdp.setName("some-new-name");
+                shouldRejectUpdate(zone1, existingIdp, HttpStatus.UNPROCESSABLE_ENTITY);


Looks good. Leaving comments to track my progress as this is challenging to read through.

Tallicia · 2024-03-27T16:16:08Z

...org/cloudfoundry/identity/uaa/mock/providers/IdentityProviderEndpointsAliasMockMvcTests.java

+
+                    // check if the returned IdP has a redacted relying party secret
+                    assertRelyingPartySecretIsRedacted(updatedIdp);
+                }


Looks good. Leaving comments to track my progress as this is challenging to read through.

Tallicia · 2024-03-27T16:16:30Z

...org/cloudfoundry/identity/uaa/mock/providers/IdentityProviderEndpointsAliasMockMvcTests.java

+                        final String newAliasId,
+                        final String newAliasZid
+                ) throws Throwable {
+                    shouldReject_ChangingAliasPropertiesOfIdpWithAlias(newAliasId, newAliasZid, IdentityZone.getUaa(), customZone);


Looks good. Leaving comments to track my progress as this is challenging to read through.

Tallicia · 2024-03-27T16:16:36Z

...org/cloudfoundry/identity/uaa/mock/providers/IdentityProviderEndpointsAliasMockMvcTests.java

+                        final String newAliasId,
+                        final String newAliasZid
+                ) throws Throwable {
+                    shouldReject_ChangingAliasPropertiesOfIdpWithAlias(newAliasId, newAliasZid, customZone, IdentityZone.getUaa());


Looks good. Leaving comments to track my progress as this is challenging to read through.

Tallicia · 2024-03-27T16:17:11Z

...org/cloudfoundry/identity/uaa/mock/providers/IdentityProviderEndpointsAliasMockMvcTests.java

-                );
+                @Test
+                void shouldReject_OnlyAliasZidSetToNull_UaaToCustomZone() throws Throwable {
+                    shouldReject_OnlyAliasZidSetToNull(IdentityZone.getUaa(), customZone);


Looks good. Leaving comments to track my progress as this is challenging to read through.

Tallicia · 2024-03-27T16:17:16Z

...org/cloudfoundry/identity/uaa/mock/providers/IdentityProviderEndpointsAliasMockMvcTests.java

-                assertThat(originalIdp.getAliasZid()).isNotBlank();
+                @Test
+                void shouldReject_OnlyAliasZidSetToNull_CustomToUaaZone() throws Throwable {
+                    shouldReject_OnlyAliasZidSetToNull(customZone, IdentityZone.getUaa());


Looks good. Leaving comments to track my progress as this is challenging to read through.

Tallicia · 2024-03-27T16:17:33Z

...org/cloudfoundry/identity/uaa/mock/providers/IdentityProviderEndpointsAliasMockMvcTests.java

-                shouldReject_OnlyAliasZidSetToNull(customZone, IdentityZone.getUaa());
-            }
+                    originalIdp.setAliasId(null);
+                    shouldRejectUpdate(zone1, originalIdp, HttpStatus.UNPROCESSABLE_ENTITY);


Looks good. Leaving comments to track my progress as this is challenging to read through.

Tallicia · 2024-03-27T16:17:40Z

...org/cloudfoundry/identity/uaa/mock/providers/IdentityProviderEndpointsAliasMockMvcTests.java

-                shouldReject_OnlyAliasZidSetToNull(IdentityZone.getUaa(), customZone);
-            }
+                    assertThat(originalIdp.getAliasId()).isNotBlank();
+                    assertThat(originalIdp.getAliasZid()).isNotBlank();


Looks good. Leaving comments to track my progress as this is challenging to read through.

Tallicia · 2024-03-27T16:19:04Z

...org/cloudfoundry/identity/uaa/mock/providers/IdentityProviderEndpointsAliasMockMvcTests.java

+    private IdentityProvider<?> updateIdpViaDb(final String zoneId, final IdentityProvider<?> idp) {
+        final JdbcIdentityProviderProvisioning identityProviderProvisioning = webApplicationContext
+                .getBean(JdbcIdentityProviderProvisioning.class);
+        return identityProviderProvisioning.update(idp, zoneId);


Looks good. Leaving comments to track my progress as this is challenging to read through.

Tallicia · 2024-03-27T16:19:36Z

...org/cloudfoundry/identity/uaa/mock/providers/IdentityProviderEndpointsAliasMockMvcTests.java

+                    assertThat(originalIdp.getAliasZid()).isNotBlank();
+
+                    originalIdp.setAliasZid(null);
+                    shouldRejectUpdate(zone1, originalIdp, HttpStatus.UNPROCESSABLE_ENTITY);


Looks good. Leaving comments to track my progress as this is challenging to read through.

Tallicia

I believe I have covered all the changes, but they are very difficult to read through. I would highly suggest keeping PRs separated between Added New tests and Updating/Refactoring existing tests.

cf-gitbot added the unscheduled label Feb 9, 2024

adrianhoelzl-sap requested review from Tallicia, swalchemist and a team February 9, 2024 08:47

bruce-ricard reviewed Feb 26, 2024

View reviewed changes

adrianhoelzl-sap requested a review from bruce-ricard February 27, 2024 09:38

adrianhoelzl-sap closed this Mar 19, 2024

adrianhoelzl-sap force-pushed the feature/alias-id-and-alias-zid-for-identity-providers-further-integration-tests branch from be1add2 to 115bad4 Compare March 19, 2024 12:27

cf-gitbot removed the unscheduled label Mar 19, 2024

Add tests for read operation

d0b2016

adrianhoelzl-sap reopened this Mar 19, 2024

cf-gitbot added the unscheduled label Mar 19, 2024

adrianhoelzl-sap marked this pull request as draft March 19, 2024 12:33

adrianhoelzl-sap added 13 commits March 19, 2024 13:48

Add handling for tests assessing dangling references to shouldRejectU…

d7040f3

…pdate method

Add updateIdpViaDb method

57096e9

Add tests: update should be rejected if the entity has no existing al…

26766e4

…ias and the aliasId is set in the update payload (valid for enabled and disabled alias feature)

Add tests: update should ignore alias if the original IdP has an empt…

f242128

…y aliasId

Add tests: update should ignore dangling reference and break referenc…

77ff7b8

…e to alias in existing IdP if alias feature is disabled

Add tests: update should reject if alias feature disabled, no alias e…

f464fa6

…xists and the update payload contains a non-empty aliasZid

Move test Update -> AliasFeatureDisabled -> shouldReject_OtherPropert…

ce4a41e

…iesChangedWhileAliasPropertiesUnchanged to nested class ExistingAlias

Move test Update -> AliasFeatureDisabled -> shouldAccept_SetOnlyAlias…

70611bc

…PropertiesToNull to nested class ExistingAlias

Move test Update -> AliasFeatureDisabled -> shouldAccept_SetAliasProp…

85bee2e

…ertiesToNullAndChangeOtherProperties to nested class ExistingAlias

Move test Update -> AliasFeatureDisabled -> shouldAccept_ShouldIgnore…

688261e

…AliasIdOfExistingIdpMissing to nested class ExistingAlias

Move test Update -> AliasFeatureDisabled -> shouldAccept_ShouldIgnore…

2f8ae6f

…DanglingReference to nested class ExistingAlias

Move test Update -> AliasFeatureDisabled -> shouldReject_OnlyAliasIdS…

98d9ab1

…etToNull to nested class ExistingAlias

Move test Update -> AliasFeatureDisabled -> shouldReject_OnlyAliasZid…

cd884e0

…SetToNull to nested class ExistingAlias

Tallicia reviewed Mar 27, 2024

View reviewed changes

Tallicia approved these changes Mar 27, 2024

View reviewed changes

Tallicia added the accepted Accepted the issue label Mar 27, 2024

adrianhoelzl-sap merged commit d6b85bc into develop Mar 28, 2024
20 checks passed

adrianhoelzl-sap deleted the feature/alias-id-and-alias-zid-for-identity-providers-further-integration-tests branch March 28, 2024 08:18

cf-gitbot removed unscheduled in progress accepted Accepted the issue labels Mar 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Further Integration Tests for Alias Identity Providers Feature #2722

Further Integration Tests for Alias Identity Providers Feature #2722

adrianhoelzl-sap commented Feb 9, 2024

cf-gitbot commented Feb 9, 2024

bruce-ricard left a comment

bruce-ricard Feb 26, 2024

adrianhoelzl-sap Feb 27, 2024

bruce-ricard Mar 6, 2024

Tallicia commented Mar 18, 2024

cf-gitbot commented Mar 19, 2024

adrianhoelzl-sap commented Mar 19, 2024

Tallicia Mar 27, 2024

Tallicia Mar 27, 2024

Tallicia Mar 27, 2024

Tallicia Mar 27, 2024

Tallicia Mar 27, 2024

Tallicia Mar 27, 2024

Tallicia Mar 27, 2024

Tallicia Mar 27, 2024

Tallicia Mar 27, 2024

Tallicia Mar 27, 2024

Tallicia Mar 27, 2024

Tallicia Mar 27, 2024

Tallicia Mar 27, 2024

Tallicia Mar 27, 2024

Tallicia Mar 27, 2024

Tallicia Mar 27, 2024

Tallicia Mar 27, 2024

Tallicia Mar 27, 2024

Tallicia Mar 27, 2024

Tallicia Mar 27, 2024

Tallicia Mar 27, 2024

Tallicia Mar 27, 2024

Tallicia Mar 27, 2024

Tallicia left a comment

Further Integration Tests for Alias Identity Providers Feature #2722

Further Integration Tests for Alias Identity Providers Feature #2722

Conversation

adrianhoelzl-sap commented Feb 9, 2024

cf-gitbot commented Feb 9, 2024

bruce-ricard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tallicia commented Mar 18, 2024

cf-gitbot commented Mar 19, 2024

adrianhoelzl-sap commented Mar 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tallicia left a comment

Choose a reason for hiding this comment