Skip to content

chore: Flaky test in AuthenticationServiceTest #35538

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
abhvsn merged 1 commit into from
Aug 8, 2024
Merged

chore: Flaky test in AuthenticationServiceTest #35538

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions ...th-server/src/main/java/com/appsmith/server/solutions/ce/AuthenticationServiceCEImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,8 +140,8 @@ public Mono<String> getAuthorizationCodeURLForGenericOAuth2(
})
.switchIfEmpty(Mono.error(
new AppsmithException(AppsmithError.NO_RESOURCE_FOUND, FieldName.DATASOURCE, datasourceId)))
.flatMap(this::validateRequiredFieldsForGenericOAuth2)
.zipWith(Mono.zip(workspaceIdMono, trueEnvironmentIdCached, newPageMono))
.flatMap(ds -> this.validateRequiredFieldsForGenericOAuth2(ds)
.zipWith(Mono.zip(workspaceIdMono, trueEnvironmentIdCached, newPageMono)))
Comment on lines +143 to +144
Copy link
Contributor

@nsarupr nsarupr Aug 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How will this change be any different than the execution which is already present.
The mono from .flatMap(this::validateRequiredFieldsForGenericOAuth2) was earlier being zipped with (Mono.zip(workspaceIdMono, trueEnvironmentIdCached, newPageMono)) which would mean that both these Monos will be executing in parallel.

With the new change

.flatMap(ds -> this.validateRequiredFieldsForGenericOAuth2(ds)
    .zipWith(Mono.zip(workspaceIdMono, trueEnvironmentIdCached, newPageMono)))

we are still doing the same thing.

Should we use something like:

.flatMap(this::validateRequiredFieldsForGenericOAuth2)
    .zipWhen(ds -> Mono.zip(workspaceIdMono, trueEnvironmentIdCached, newPageMono))

Copy link
Contributor Author

@abhvsn abhvsn Aug 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nsarupr the error is popping up because newPageMono is getting executed alongside of datasourceMonoCached. With the updated change we are making sure Monos will be executed within the flatMap which was not the case earlier.

Earlier:

.flatMap(this::validateRequiredFieldsForGenericOAuth2)
.zipWith(Mono.zip(workspaceIdMono, trueEnvironmentIdCached, newPageMono))

After changes:

.flatMap(ds -> this.validateRequiredFieldsForGenericOAuth2(ds)
    .zipWith(Mono.zip(workspaceIdMono, trueEnvironmentIdCached, newPageMono))
)

Also zipWhen also works but thought, all the zipped entities should be within the same flatMap hence opted for this approach.

This also works:

.flatMap(this::validateRequiredFieldsForGenericOAuth2)
.zipWhen(ds -> Mono.zip(workspaceIdMono, trueEnvironmentIdCached, newPageMono))

Copy link
Contributor

@NilanshBansal NilanshBansal Aug 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@abhvsn should we use zipWhen() for a cleaner code and better readability here?

Copy link
Contributor Author

@abhvsn abhvsn Aug 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As stated I would not prefer that considering we are not using the returned value in zipWhen.

.flatMap(tuple2 -> {
DatasourceStorage datasourceStorage = tuple2.getT1();
String workspaceId = tuple2.getT2().getT1();
Expand Down