-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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.
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
chore: Added capability of running ITs on maven #38354
Conversation
WalkthroughThis pull request introduces modifications to the Appsmith server's Git-related components, focusing on artifact type management and test configuration. The changes involve updating the Changes
Sequence DiagramsequenceDiagram
participant GitContext
participant GitServerInitializerExtension
participant ArtifactService
GitContext->>GitContext: Store artifactType in contextStore
GitServerInitializerExtension->>ArtifactService: createOrUpdateSshKeyPair(artifactType, artifactId, keyType)
Possibly related PRs
Suggested labels
Suggested reviewers
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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/server/pom.xml (1)
80-103
: Adding multiple test sources via build-helper-maven-plugin
Definingsrc/test/java
,src/test/it
, andsrc/test/utils
allows a cleaner test organization. This step helps maintain clarity between unit, integration, and utility tests.Consider adding comments or documentation explaining the semantic purpose of each test directory.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/server/appsmith-server/src/test/it/com/appsmith/server/git/templates/contexts/GitContext.java
(1 hunks)app/server/appsmith-server/src/test/utils/com/appsmith/server/git/GitServerInitializerExtension.java
(3 hunks)app/server/pom.xml
(3 hunks)
🔇 Additional comments (8)
app/server/appsmith-server/src/test/it/com/appsmith/server/git/templates/contexts/GitContext.java (1)
28-28
: Storing artifactType in the context store
This additional context store entry helps streamline the retrieval of artifactType
in the extension. The change is minimal and makes sense in ensuring consistent artifact-type handling downstream.
app/server/appsmith-server/src/test/utils/com/appsmith/server/git/GitServerInitializerExtension.java (3)
4-5
: Replacing ApplicationService with ArtifactService
Switching to ArtifactService
clearly aligns the implementation with artifact-centric operations. This change appears consistent with the new workflow.
Also applies to: 40-40
59-59
: ArtifactType usage
Retrieving ArtifactType
from the parent context ensures the appropriate artifact type is mapped before creating the SSH key pair. This is a good design approach.
67-67
: Creating or updating SSH key pair
The createOrUpdateSshKeyPair
call on artifactService
is well-structured. Ensure the returned mono (i.e., gitAuthMono
) handles errors gracefully in upstream code.
app/server/pom.xml (4)
46-48
: Granular test skipping flags
Introducing <skipITs>
, <skipTests>
, and <skipUTs>
provides fine control over test execution. This meets the PR objective of distinguishing UT from IT runs.
114-116
: Surefire test configuration
Setting <testSourceDirectory>
to src/test/java
and using <skipTests>${skipUTs}</skipTests>
is a direct and effective way to skip only unit tests when needed.
117-129
: JUnit Jupiter engine dependency for unit tests
It’s good to see version consistency across JUnit dependencies. Excluding junit-platform-commons
is consistent with your test framework strategy.
130-148
: Failsafe integration test configuration
Pointing <testSourceDirectory>
to src/test/it
and referencing <skipITs>
matches the intended separation of UTs and ITs. This streamlines build management.
@@ -40,7 +37,7 @@ | |||
public class GitServerInitializerExtension implements BeforeAllCallback, BeforeEachCallback, AfterEachCallback, AfterAllCallback { | |||
|
|||
@Autowired | |||
ApplicationService applicationService; | |||
ArtifactService artifactService; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be parameterized?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably not in the scope of this PR, however something to note down
Description
Enables running UTs and ITs as desired on maven. The skipTests command continues to skip all tests.
To run only UTs, use:
mvn test
To run only ITs, use:
mvn verify -DskipUTs=true
Automation
/ok-to-test tags="@tag.Git"
Summary by CodeRabbit
New Features
artifactType
with theextensionContext
.Bug Fixes
Documentation
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12483043596
Commit: 9b65433
Cypress dashboard.
Tags:
@tag.Git
Spec:
Tue, 24 Dec 2024 15:24:20 UTC