test: Git integration tests #38337
test: Git integration tests #38337
Conversation
WalkthroughThis pull request introduces a comprehensive set of changes to the Git-related testing infrastructure in the Appsmith server. The modifications focus on enhancing the testing capabilities for Git operations, including new utility classes, test contexts, and extensions that support more robust and flexible integration testing of Git workflows for artifacts. Changes
Sequence DiagramsequenceDiagram
participant Test as Integration Test
participant GitContext as Git Context
participant GitServer as Git Server
participant ArtifactBuilder as Artifact Builder
Test->>GitContext: Initialize Context
GitContext->>ArtifactBuilder: Create Workspace
ArtifactBuilder-->>GitContext: Return Workspace ID
GitContext->>GitServer: Create Repository
GitServer-->>GitContext: Repository Created
Test->>GitContext: Perform Git Operations
Test->>GitContext: Validate Outcomes
Suggested labels
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
skip-changelog
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (11)
app/server/appsmith-server/src/test/it/com/appsmith/server/git/GitBranchesIT.java (3)
167-170: Resolve the TODO comments.
There are follow-up TODO items to move filePath variables to be relative and to consider using the controller layer. Address these to maintain clarity and ensure test consistency.
217-227: Avoid using Thread.sleep in tests.
Polling with Thread.sleep can introduce flaky behavior and unnecessary delays. Consider using reactive constructs (e.g., StepVerifier or Mono/Flux time-bound methods) or a condition-based wait for improved reliability.
431-436: Automate branch checkout.
Manually performing a checkout before deleting a branch reflects a potential usability gap. Consider implementing a logic that automatically checks out a different branch or warns the user before attempting deletion.app/server/appsmith-server/src/test/utils/com/appsmith/server/git/GitTestUtils.java (1)
14-19: Prepare for artifact type expansions.
Currently, getArtifactSpecificUtils() always returns gitApplicationTestUtils. If future artifact types are introduced, additional logic or classes may be necessary.app/server/appsmith-server/src/test/it/com/appsmith/server/git/templates/contexts/GitContext.java (2)
33-35: Use invocationIndex in displayName for clarity.
Appending invocationIndex in the returned displayName can aid in debugging test iterations.public String getDisplayName(int invocationIndex) { - return fileName; + return fileName + " [#" + invocationIndex + "]"; }
59-63: Refine parameter resolution.
Currently, supportsParameter always returns true, leading to potential confusion for parameters the extension does not expect. Narrow the condition or add additional checks.app/server/appsmith-server/src/test/utils/com/appsmith/server/git/ArtifactBuilderExtension.java (1)
95-103: Ensure consistent cleanup in case of partial failures.
After deleting applications, consider verifying that the workspace itself is archived successfully, especially when encountering unexpected errors mid-flow.app/server/appsmith-server/src/test/utils/com/appsmith/server/git/GitServerInitializerExtension.java (2)
59-96: Add error handling for repo creation.
When attaching SSH keys and creating the repository, if the repository already exists or the API call fails, consider raising a clearer error for debugging.
110-112: Stop container only if running check is reliable.
Linger checks might be helpful to ensure the Git server has had time to shut down completely, preventing possible conflicts in subsequent tests if the container lingers.app/server/appsmith-server/src/test/utils/com/appsmith/server/git/ArtifactBuilderContext.java (2)
7-8: Add interface documentation.Consider adding Javadoc to describe the purpose of this interface and its role in Git integration testing.
+/** + * Context for building artifacts during Git integration tests. + * Extends ExtensionContext to provide artifact-specific metadata and management capabilities + * within test execution contexts. + */ public interface ArtifactBuilderContext extends ExtensionContext {
15-21: Consider adding parameter validation.The setter methods should validate their input parameters to prevent null or empty strings from being set.
- void setWorkspaceId(String workspaceId); + /** + * Sets the workspace ID for the artifact context. + * @param workspaceId must not be null or empty + * @throws IllegalArgumentException if workspaceId is null or empty + */ + void setWorkspaceId(String workspaceId); - void setArtifactId(String artifactId); + /** + * Sets the artifact ID for the context. + * @param artifactId must not be null or empty + * @throws IllegalArgumentException if artifactId is null or empty + */ + void setArtifactId(String artifactId);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
app/server/appsmith-server/src/main/java/com/appsmith/server/domains/ce/ArtifactCE.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/git/common/CommonGitServiceCEImpl.java(1 hunks)app/server/appsmith-server/src/test/it/com/appsmith/server/git/GitBranchesIT.java(1 hunks)app/server/appsmith-server/src/test/it/com/appsmith/server/git/templates/contexts/GitContext.java(1 hunks)app/server/appsmith-server/src/test/it/com/appsmith/server/git/templates/providers/GitBranchesTestTemplateProvider.java(1 hunks)app/server/appsmith-server/src/test/it/com/appsmith/server/git/templates/providers/ce/GitBranchesTestTemplateProviderCE.java(1 hunks)app/server/appsmith-server/src/test/utils/com/appsmith/server/git/ArtifactBuilderContext.java(1 hunks)app/server/appsmith-server/src/test/utils/com/appsmith/server/git/ArtifactBuilderExtension.java(1 hunks)app/server/appsmith-server/src/test/utils/com/appsmith/server/git/GitArtifactTestUtils.java(1 hunks)app/server/appsmith-server/src/test/utils/com/appsmith/server/git/GitServerInitializerExtension.java(1 hunks)app/server/appsmith-server/src/test/utils/com/appsmith/server/git/GitTestUtils.java(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/server/appsmith-server/src/test/it/com/appsmith/server/git/templates/providers/GitBranchesTestTemplateProvider.java
🔇 Additional comments (10)
app/server/appsmith-server/src/test/it/com/appsmith/server/git/GitBranchesIT.java (2)
346-349: Validate error message handling.
Ensure that all error messages (e.g., “Cannot commit to protected branch”) are localized or handled consistently if there is any plan for internationalization or user-facing logs.
493-495: Investigate the persistent diff after discard.
A repeated diff is being triggered by an “on page load” event. Confirm whether this is expected or a bug to avoid false positives in test results.
app/server/appsmith-server/src/main/java/com/appsmith/server/domains/ce/ArtifactCE.java (1)
16-17: Method addition looks good.
The setter method for artifact name aligns well with existing getters. This provides better control over artifact properties.
app/server/appsmith-server/src/test/it/com/appsmith/server/git/templates/providers/ce/GitBranchesTestTemplateProviderCE.java (1)
14-31: Parameterized test provider looks good.
This test template provider correctly sets up GitContext for parameterized testing. No issues found.
app/server/appsmith-server/src/test/utils/com/appsmith/server/git/GitArtifactTestUtils.java (1)
28-51: Handle plugin lookup failures.
If the “restapi-plugin” is unavailable, pluginService.findByPackageName(...) may return null, leading to potential NullPointerExceptions. Consider defensive checks or fallback mechanisms.
+if (plugin == null) {
+ return Mono.error(new IllegalStateException("Plugin 'restapi-plugin' not found"));
+}app/server/appsmith-server/src/test/it/com/appsmith/server/git/templates/contexts/GitContext.java (1)
22-30: Reuse of ExtensionContext.Store is good; carefully consider impacts of writing to the same namespace.
Storing data in the same namespace as ArtifactBuilderExtension is logically consistent. Nonetheless, ensure no collisions if multiple extensions use the same keys.
app/server/appsmith-server/src/test/utils/com/appsmith/server/git/ArtifactBuilderExtension.java (1)
72-83: Consider concurrency checks for artifact creation.
While you create and import a new artifact in the workspace, watch for concurrency scenarios where multiple tests create artifacts simultaneously.
app/server/appsmith-server/src/test/utils/com/appsmith/server/git/GitServerInitializerExtension.java (1)
48-52: Confirm container readiness in test environments.
While the container’s readiness check is set to wait on port 4200, consider handling slow startups gracefully and logging extended delays if the container fails to start.
app/server/appsmith-server/src/main/java/com/appsmith/server/git/common/CommonGitServiceCEImpl.java (1)
2190-2200: Proper lock release on failed branch deletion is commendable.
Returning after calling releaseFileLock ensures no leftover locks remain when the branch cannot be deleted. This prevents resource contention in subsequent operations.
app/server/appsmith-server/src/test/utils/com/appsmith/server/git/ArtifactBuilderContext.java (1)
1-6: LGTM! Clean imports and proper package structure.
| void test(GitContext gitContext, ExtensionContext extensionContext) throws IOException, GitAPIException, InterruptedException { | ||
|
|
||
| ExtensionContext.Store contextStore = extensionContext.getStore(ExtensionContext.Namespace.create(ArtifactBuilderExtension.class)); | ||
| String artifactId = contextStore.get(FieldName.ARTIFACT_ID, String.class); | ||
|
|
||
| GitConnectDTO connectDTO = new GitConnectDTO(); | ||
| connectDTO.setRemoteUrl(gitServerInitializerExtension.getGitSshUrl("test" + artifactId)); | ||
| GitProfile gitProfile = new GitProfile("foo bar", "[email protected]", null); | ||
| connectDTO.setGitProfile(gitProfile); | ||
|
|
||
| // TODO: | ||
| // - Move the filePath variable to be relative, so that template name and repo name is prettier | ||
| // - Is it possible to use controller layer here? Might help with also including web filters in IT | ||
| Artifact artifact = commonGitService.connectArtifactToGit(artifactId, connectDTO, ORIGIN, gitContext.getArtifactType()) | ||
| .block(); | ||
|
|
||
| assertThat(artifact).isNotNull(); | ||
|
|
||
| ArtifactType artifactType = artifact.getArtifactType(); | ||
| GitArtifactMetadata artifactMetadata = artifact.getGitArtifactMetadata(); | ||
| GitArtifactHelper<?> artifactHelper = gitArtifactHelperResolver.getArtifactHelper(artifactType); | ||
| Path repoSuffix = artifactHelper.getRepoSuffixPath( | ||
| artifact.getWorkspaceId(), | ||
| artifactMetadata.getDefaultArtifactId(), | ||
| artifactMetadata.getRepoName()); | ||
|
|
||
| // Auto-commit should be turned on by default | ||
| assertThat(artifactMetadata.getAutoCommitConfig().getEnabled()).isTrue(); | ||
|
|
||
| Path path = Path.of(gitServiceConfig.getGitRootPath()).resolve(repoSuffix); | ||
| String branch; | ||
| ObjectId topOfCommits; | ||
|
|
||
| try (Git git = Git.open(path.toFile())) { | ||
| branch = git.log().getRepository().getBranch(); | ||
| assertThat(branch).isEqualTo(artifactMetadata.getBranchName()); | ||
|
|
||
| // Assert only single system generated commit exists on FS | ||
| Iterable<RevCommit> commits = git.log().call(); | ||
| Iterator<RevCommit> commitIterator = commits.iterator(); | ||
| assertThat(commitIterator.hasNext()).isTrue(); | ||
|
|
||
| RevCommit firstCommit = commitIterator.next(); | ||
| assertThat(firstCommit.getFullMessage()).isEqualTo(DEFAULT_COMMIT_MESSAGE + GitDefaultCommitMessage.CONNECT_FLOW.getReason()); | ||
| topOfCommits = firstCommit.getId(); | ||
|
|
||
| assertThat(commitIterator.hasNext()).isFalse(); | ||
|
|
||
| // Assert that git directory is clean | ||
| Status status = git.status().call(); | ||
| assertThat(status.isClean()).isTrue(); | ||
| } | ||
|
|
||
| // Assert that the artifact does have auto-commit requirements, and auto-commit gets initiated | ||
| AutoCommitResponseDTO autoCommitResponseDTO = autoCommitService.autoCommitApplication(artifactId).block(); | ||
|
|
||
| assertThat(autoCommitResponseDTO).isNotNull(); | ||
| AutoCommitResponseDTO.AutoCommitResponse autoCommitProgress = autoCommitResponseDTO.getAutoCommitResponse(); | ||
| assertThat(autoCommitProgress).isEqualTo(AutoCommitResponseDTO.AutoCommitResponse.PUBLISHED); | ||
|
|
||
| // Wait for auto-commit to complete | ||
| // This should not take more than 2 seconds, we're checking every 500 ms | ||
| long startTime = System.currentTimeMillis(), currentTime = System.currentTimeMillis(); | ||
| while (!autoCommitProgress.equals(AutoCommitResponseDTO.AutoCommitResponse.IDLE)) { | ||
| Thread.sleep(500); | ||
| if (currentTime - startTime > 2000) { | ||
| fail("Auto-commit took too long"); | ||
| } | ||
| autoCommitProgress = getAutocommitProgress(artifactId, artifact, artifactMetadata); | ||
| currentTime = System.currentTimeMillis(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider splitting the test for better maintainability.
This single test method is quite long and covers a broad range of scenarios. Splitting it into multiple focused tests would improve readability, reduce complexity, and make it easier to locate and isolate failures.
| private Mono<? extends ArtifactExchangeJson> createArtifactJson(String filePath, Class<? extends ArtifactExchangeJson> exchangeJsonType) throws IOException { | ||
|
|
||
| ClassPathResource classPathResource = new ClassPathResource(filePath); | ||
|
|
||
| String artifactJson = classPathResource.getContentAsString(Charset.defaultCharset()); | ||
|
|
||
| ArtifactExchangeJson artifactExchangeJson = | ||
| objectMapper.copy().disable(MapperFeature.USE_ANNOTATIONS).readValue(artifactJson, exchangeJsonType); | ||
|
|
||
| return jsonSchemaMigration.migrateArtifactExchangeJsonToLatestSchema(artifactExchangeJson, null, null); | ||
| } |
There was a problem hiding this comment.
Validate artifact JSON before migration.
Consider adding pre-validation steps to handle malformed or incompatible artifact files gracefully, protecting against potential I/O or parsing exceptions at runtime.
| ArtifactType getArtifactType(); | ||
|
|
||
| Class<? extends ArtifactExchangeJson> getArtifactJsonType(); | ||
|
|
||
| String getArtifactJsonPath(); |
There was a problem hiding this comment.
💡 Codebase verification
Add Javadoc to clarify these critical artifact interface methods
These methods are widely used throughout the codebase for Git operations, artifact handling, and import/export functionality. Documentation is essential as they are part of a core interface.
getArtifactType(): Returns the type of artifact (e.g., Application) being processedgetArtifactJsonType(): Specifies the JSON structure class for artifact serializationgetArtifactJsonPath(): Defines the file path for storing the artifact's JSON representation
🔗 Analysis chain
Document artifact-related methods.
Add Javadoc to clarify the purpose of these methods. Also, verify the implementation in test classes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find implementations to ensure proper usage
ast-grep --pattern 'class $_ implements ArtifactBuilderContext {
$$$
getArtifactType() {
$$$
}
$$$
}'
Length of output: 115
Script:
#!/bin/bash
# Let's try a different approach to find implementations and usages
# First, find all files that might contain the interface or implementations
rg -l "ArtifactBuilderContext"
# Then look for the actual interface definition and its usages
rg -A 5 "interface ArtifactBuilderContext"
# Search for implementations with a simpler pattern
rg -A 5 "implements ArtifactBuilderContext"
# Look for usages of these methods
rg "getArtifactType\(\)"
rg "getArtifactJsonType\(\)"
rg "getArtifactJsonPath\(\)"
Length of output: 13463
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="@tag.Git"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12479113304
Commit: bebfa2a
Cypress dashboard.
Tags:
@tag.GitSpec:
Tue, 24 Dec 2024 09:11:23 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores