chore: setup for server test changes and fixing few #33847
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
vivonk
commented
May 30, 2024
| @@ -56,6 +57,8 @@ public enum AuthenticationStatus { | |||
| @JsonView(Views.Internal.class) | |||
| AuthenticationResponse authenticationResponse; | |||
|
|
|||
| @JsonView(Views.Public.class) | |||
| @Transient | |||
| public Mono<Boolean> hasExpired() { | |||
There was a problem hiding this comment.
persistence fails so making is transient
vivonk
commented
May 30, 2024
| @@ -141,14 +139,5 @@ public void updateForBulkWriteOperation() { | |||
| this.setUpdatedAt(Instant.now()); | |||
| } | |||
|
|
|||
| @PrePersist | |||
There was a problem hiding this comment.
not needed anymore as we are assigning id now in save method itself
vivonk
commented
May 30, 2024
| @@ -32,6 +32,7 @@ public enum TableType { | |||
|
|
|||
| @Data | |||
| @AllArgsConstructor | |||
| @NoArgsConstructor | |||
There was a problem hiding this comment.
Needed as serde was failing in few tests
vivonk
commented
May 30, 2024
| @@ -256,7 +256,7 @@ private Mono<Application> createSuffixedApplication(Application application, Str | |||
| return super.create(application).onErrorResume(DataIntegrityViolationException.class, error -> { | |||
| if (error.getMessage() != null | |||
| // Catch only if error message contains workspace_app_deleted_git_application_metadata mongo error | |||
| && error.getMessage().contains("workspace_app_deleted_git_application_metadata")) { | |||
| && (error.getMessage().contains("u_workspace_app"))) { | |||
There was a problem hiding this comment.
index name has changed
There was a problem hiding this comment.
Should we keep the nomenclature same for the indexes? That way we don't have to look into different branches if we face some issues in future.
vivonk
commented
May 30, 2024
| Set<Policy> policies = mapObject(policiesJson, new TypeReference<Set<Policy>>() {}); | ||
| TenantConfiguration tenantConfiguration = |
There was a problem hiding this comment.
updated after found yesterday that tenantconfiguration was setting up null if not provided here
There was a problem hiding this comment.
Let's add this in the docs as well seems to be something which can be missed by other devs.
vivonk
commented
May 30, 2024
| @@ -26,15 +27,16 @@ private void addAnonymousUser() throws JsonProcessingException { | |||
| return; | |||
| } | |||
| String insertUserQuery = | |||
| "INSERT INTO \"user\" (id, email, name, current_workspace_id, workspace_ids, is_anonymous, tenant_id, is_system_generated, created_at, updated_at) VALUES (gen_random_uuid(), ?, ?, ?, cast(? as jsonb), ?, ?, true, now() ,now())"; | |||
| "INSERT INTO \"user\" (id, email, name, current_workspace_id, workspace_ids, is_anonymous, tenant_id, source, is_system_generated, created_at, updated_at) VALUES (gen_random_uuid(), ?, ?, ?, cast(? as jsonb), ?, ?, ?, true, now() ,now())"; | |||
There was a problem hiding this comment.
login source was missing earlier
vivonk
commented
May 30, 2024
| public Mono<T> save(T entity) { | ||
| return Mono.fromSupplier(() -> repository.save(entity)).subscribeOn(Schedulers.boundedElastic()); | ||
| return Mono.fromSupplier(() -> { |
There was a problem hiding this comment.
updates save method so that we can deal with save failures ourselves
vivonk
commented
May 30, 2024
| if (error.getMessage() != null | ||
| && error.getMessage().contains("workspace_datasource_deleted_compound_index") | ||
| && error.getMessage().contains("u_workspace_datasource") |
vivonk
commented
May 30, 2024
| @@ -6,6 +6,10 @@ create unique index if not exists u_email on password_reset_token(email); | |||
| create unique index if not exists plugin_name_package_name_version_index on plugin(plugin_name, package_name, version); | |||
| create unique index if not exists u_user_id on user_data(user_id); | |||
| create unique index if not exists u_workspace_datasource_deleted on datasource(workspace_id, name, deleted_at); | |||
| CREATE UNIQUE INDEX if not exists u_workspace_datasource ON datasource (workspace_id, name) WHERE deleted_at IS NULL; | |||
| create unique index if not exists u_workspace_app_deleted on application(workspace_id, name, deleted_at) where git_application_metadata is null; | |||
| create unique index if not exists u_workspace_app on application(workspace_id, name) where deleted_at is null and git_application_metadata is null; | |||
There was a problem hiding this comment.
new index addition based on the fact that null values aren't treated same in postgres and we need different indexes if a value part of index is null
There was a problem hiding this comment.
Can you share more details on this, just a ref should be enough?
vivonk
commented
May 30, 2024
|
|
||
| @Profile("test") | ||
| @Configuration | ||
| public class TestDatabaseConfig { |
There was a problem hiding this comment.
config specific to tests
There was a problem hiding this comment.
Let's add the javadoc comment to define the purpose, and also provide the reason why this is needed in tests against the production environment.
abhvsn
reviewed
May 30, 2024
| @@ -256,7 +256,7 @@ private Mono<Application> createSuffixedApplication(Application application, Str | |||
| return super.create(application).onErrorResume(DataIntegrityViolationException.class, error -> { | |||
| if (error.getMessage() != null | |||
| // Catch only if error message contains workspace_app_deleted_git_application_metadata mongo error | |||
| && error.getMessage().contains("workspace_app_deleted_git_application_metadata")) { | |||
| && (error.getMessage().contains("u_workspace_app"))) { | |||
There was a problem hiding this comment.
Should we keep the nomenclature same for the indexes? That way we don't have to look into different branches if we face some issues in future.
| @@ -28,7 +30,7 @@ public ResolvedMigration getResolvedMigration(Configuration config, StatementInt | |||
| public final void migrate(Context context) throws Exception { | |||
| JdbcTemplate jdbcTemplate = | |||
| new JdbcTemplate(new SingleConnectionDataSource(context.getConnection(), true), false); | |||
|
|
|||
| log.info("Running migration: {}", this.getClass().getSimpleName()); | |||
There was a problem hiding this comment.
What are the expected logs here? Does logs from flyway not sufficient for us.
| Set<Policy> policies = mapObject(policiesJson, new TypeReference<Set<Policy>>() {}); | ||
| TenantConfiguration tenantConfiguration = |
There was a problem hiding this comment.
Let's add this in the docs as well seems to be something which can be missed by other devs.
| if (error.getMessage() != null | ||
| && error.getMessage().contains("workspace_datasource_deleted_compound_index") | ||
| && error.getMessage().contains("u_workspace_datasource") |
...ver/appsmith-server/src/main/java/com/appsmith/server/services/ce/MockDataServiceCEImpl.java
Outdated
Show resolved
Hide resolved
| @@ -159,6 +159,7 @@ public void testGetMockDataSets() { | |||
|
|
|||
| @Test | |||
| @WithUserDetails(value = "api_user") | |||
| @Order(1) | |||
There was a problem hiding this comment.
Any specific need for this ordering? We ideally want each test to be self sufficient.
| @@ -48,7 +48,7 @@ | |||
|
|
|||
| import static org.assertj.core.api.Assertions.assertThat; | |||
|
|
|||
| @ExtendWith(SpringExtension.class) | |||
| @ExtendWith(AfterAllCleanUpExtension.class) | |||
| @SpringBootTest | |||
| public class AuthenticationServiceTest { | |||
There was a problem hiding this comment.
Don't we require @DirtiesContext here?
| @@ -49,11 +49,10 @@ | |||
| import static org.mockito.Mockito.any; | |||
| import static org.mockito.Mockito.doReturn; | |||
|
|
|||
| @ExtendWith(SpringExtension.class) | |||
| @ExtendWith(AfterAllCleanUpExtension.class) | |||
| @SpringBootTest | |||
| @Slf4j | |||
| public class DatasourceStructureSolutionTest { | |||
There was a problem hiding this comment.
Same here @DirtiesContext is missing.
| @@ -61,10 +61,9 @@ | |||
| import static org.assertj.core.api.Assertions.assertThat; | |||
|
|
|||
| @Slf4j | |||
| @ExtendWith(SpringExtension.class) | |||
| @ExtendWith(AfterAllCleanUpExtension.class) | |||
| @SpringBootTest | |||
| public class PartialExportServiceTest { | |||
There was a problem hiding this comment.
@DirtiesContext is missing.
| @@ -91,7 +91,7 @@ | |||
| import static org.assertj.core.api.Assertions.assertThat; | |||
| import static org.mockito.ArgumentMatchers.any; | |||
|
|
|||
| @ExtendWith(SpringExtension.class) | |||
| @ExtendWith(AfterAllCleanUpExtension.class) | |||
| @SpringBootTest | |||
| @Slf4j | |||
| public class ActionExecutionSolutionCETest { | |||
Failed server tests
|
1 similar comment
Failed server tests
|
|
@abhvsn with index, names are very similar just that we have shorten as required and in case of application on |
Failed server tests
|
Failed server tests
|
abhvsn
approved these changes
May 30, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Tip
Add a TL;DR when the description is longer than 500 words or extremely technical (helps the content, marketing, and DevRel team).
Please also include relevant motivation and context. List any dependencies that are required for this change. Add links to Notion, Figma or any other documents that might be relevant to the PR.
Fixes #
Issue Numberor
Fixes
Issue URLWarning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
Automation
/ok-to-test tags=""
🔍 Cypress test results
Caution
If you modify the content in this section, you are likely to disrupt the CI result for your PR.
Communication
Should the DevRel and Marketing teams inform users about this change?