TESTING EXTERNAL SCRIPT: external merge request from Contributor #37393
Conversation
… more testcase - removed unwanted checks
…should-work-without-username-and-password' into chore/external-contribution-app-37319
WalkthroughThe changes in the pull request involve modifications to the Changes
Suggested labels
Suggested reviewers
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
app/server/appsmith-plugins/smtpPlugin/src/test/java/com/external/plugins/SmtpPluginTest.java (1)
143-157: Add a comment explaining the configuration reset.The test is well-structured, but consider adding a comment before line 157 explaining why we reset to the default SMTP configuration:
}) .verifyComplete(); + // Reset to default SMTP configuration to avoid affecting subsequent tests configureSmtpConnection(smtp);app/server/appsmith-plugins/smtpPlugin/src/main/java/com/external/plugins/SmtpPlugin.java (1)
215-232: Refactor: Set 'mail.smtp.auth' property conditionallyConsider setting the
mail.smtp.authproperty only when authentication is provided.Apply this diff to simplify the code:
prop.put("mail.transport.protocol", "smtp"); -prop.put("mail.smtp.auth", true); prop.put("mail.smtp.starttls.enable", "true"); prop.put("mail.smtp.host", endpoint.getHost()); Long port = (endpoint.getPort() == null || endpoint.getPort() < 0) ? 25 : endpoint.getPort(); prop.put("mail.smtp.port", String.valueOf(port)); prop.put("mail.smtp.ssl.trust", endpoint.getHost()); Session session; if (authentication != null && StringUtils.hasText(authentication.getUsername()) && StringUtils.hasText(authentication.getPassword())) { + prop.put("mail.smtp.auth", "true"); String username = authentication.getUsername(); String password = authentication.getPassword(); session = Session.getInstance(prop, new Authenticator() { @Override protected PasswordAuthentication getPasswordAuthentication() { return new PasswordAuthentication(username, password); } }); } else { + prop.put("mail.smtp.auth", "false"); session = Session.getInstance(prop); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
app/server/appsmith-plugins/smtpPlugin/src/main/java/com/external/plugins/SmtpPlugin.java(2 hunks)app/server/appsmith-plugins/smtpPlugin/src/test/java/com/external/plugins/SmtpPluginTest.java(3 hunks)
🔇 Additional comments (2)
app/server/appsmith-plugins/smtpPlugin/src/test/java/com/external/plugins/SmtpPluginTest.java (1)
62-77: LGTM! Well-structured container setup and refactoring.
The addition of smtpWithoutAuth container and the refactoring of the setup method improve test maintainability and coverage.
app/server/appsmith-plugins/smtpPlugin/src/main/java/com/external/plugins/SmtpPlugin.java (1)
Line range hint 268-277: Good use of specific exception handling
Catching NoSuchProviderException and AuthenticationFailedException separately provides clearer error messages.
Failed server tests
|
…ssword' of https://github.com/shanid544/appsmith into chore/external-contribution-app-37319
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
app/server/appsmith-plugins/smtpPlugin/src/test/java/com/external/plugins/SmtpPluginTest.java (1)
143-157: LGTM! Consider adding error case assertions.The happy path is well tested. Consider adding assertions for invalid configurations (e.g., wrong port) to ensure proper error handling.
Add error case test:
+@Test +public void testConnectionWithoutAuth_invalidPort() { + configureSmtpConnection(smtpWithoutAuth); + DatasourceConfiguration invalidConfig = createDatasourceConfiguration(); + invalidConfig.setAuthentication(null); + invalidConfig.setEndpoints(List.of(new Endpoint(host, 9999L))); + + StepVerifier.create(pluginExecutor.testDatasource(invalidConfig)) + .assertNext(result -> { + assertNotNull(result); + assertFalse(result.isSuccess()); + }) + .verifyComplete(); +}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
app/server/appsmith-plugins/smtpPlugin/src/main/java/com/external/plugins/SmtpPlugin.java(2 hunks)app/server/appsmith-plugins/smtpPlugin/src/test/java/com/external/plugins/SmtpPluginTest.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/server/appsmith-plugins/smtpPlugin/src/main/java/com/external/plugins/SmtpPlugin.java
🔇 Additional comments (3)
app/server/appsmith-plugins/smtpPlugin/src/test/java/com/external/plugins/SmtpPluginTest.java (3)
71-73: LGTM! Good refactoring of setup method.
Clean separation of concerns by delegating configuration to a helper method.
61-65: LGTM! Container configuration looks good.
The container is properly configured with empty credentials and a different port to avoid conflicts.
Let's verify the port availability:
#!/bin/bash
# Check if port 1025 is commonly used by other services
rg -l "1025" | grep -i "docker\|container\|config"75-78:
Consider thread safety implications.
The method modifies static fields (host and port), which could cause issues if tests are run in parallel. Consider making these fields instance-specific or using thread-local storage.
Let's check for parallel test execution configurations:
Description
Fixes #
Warning
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.Datasource"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11839915463
Commit: 7293e2c
Cypress dashboard.
Tags:
@tag.DatasourceSpec:
Thu, 14 Nov 2024 15:46:46 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Tests