chore: Disable MSSql tests as test container is unable to accept connection#36451
chore: Disable MSSql tests as test container is unable to accept connection#36451
Conversation
WalkthroughThe changes in this pull request involve the addition of the Changes
Possibly related PRs
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
the
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: 0
Outside diff range and nitpick comments (2)
app/server/appsmith-plugins/mssqlPlugin/src/test/java/com/external/plugins/MssqlGetDBSchemaTest.java (1)
26-26: Tests disabled temporarily, as expected.Class
MssqlGetDBSchemaTesthas been annotated with@Disabled, which will prevent all tests within this class from being executed. This change aligns with the pull request objective to disable the MSSql tests due to the test container being unable to accept connections.However, disabling tests should be a temporary measure. I recommend creating a follow-up task to investigate and resolve the underlying issue with the test container, so that these tests can be re-enabled. Disabling tests reduces the test coverage and may allow bugs to slip through undetected.
Do you want me to open a GitHub issue to track the task of investigating and resolving the test container issue?
app/server/appsmith-plugins/mssqlPlugin/src/test/java/com/external/plugins/MssqlPluginTest.java (1)
26-26: Remember to remove the@Disabledimport before merging.I see you've imported the
@Disabledannotation, which is typically used to temporarily disable tests. Just remember to remove this import statement before merging the code, unless you intend to disable the tests permanently.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- app/server/appsmith-plugins/mssqlPlugin/src/test/java/com/external/plugins/MssqlGetDBSchemaTest.java (2 hunks)
- app/server/appsmith-plugins/mssqlPlugin/src/test/java/com/external/plugins/MssqlPluginTest.java (2 hunks)
Additional comments not posted (2)
app/server/appsmith-plugins/mssqlPlugin/src/test/java/com/external/plugins/MssqlGetDBSchemaTest.java (1)
6-6: Good work importing the necessary annotation!The import statement for
org.junit.jupiter.api.Disabledis required to use the@Disabledannotation in the code. This is a necessary change.app/server/appsmith-plugins/mssqlPlugin/src/test/java/com/external/plugins/MssqlPluginTest.java (1)
58-58: Disabling the tests looks good for now, but let's not forget to re-enable them later.Adding the
@Disabledannotation at the class level is the right approach to temporarily disable all the tests within theMssqlPluginTestclass. This aligns with the PR objective to disable the MsSQL tests due to the connection issue with the test container.However, it's important that we don't leave the tests disabled indefinitely. Let's create a follow-up task to investigate and resolve the underlying connection issue, so that we can re-enable these tests in a future PR.
Failed server tests
|
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
app/server/appsmith-plugins/redisPlugin/src/test/java/com/external/plugins/RedisPluginTest.java (1)
43-43: Class, pay attention! The@Disabledannotation has been used to disable the entire test class.This change aligns with the PR objective to temporarily disable the MSSql tests due to the current technical limitations with the test container.
However, it's important to keep in mind that disabling the entire test class will reduce the test coverage for the Redis plugin and may lead to undetected issues in the future.
As a gentle reminder, please create a follow-up task to investigate and resolve the underlying issue with the test container. Once resolved, make sure to re-enable these tests to maintain a high level of quality and catch any potential regressions.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- app/server/appsmith-plugins/redisPlugin/src/test/java/com/external/plugins/RedisPluginTest.java (2 hunks)
Additional comments not posted (1)
app/server/appsmith-plugins/redisPlugin/src/test/java/com/external/plugins/RedisPluginTest.java (1)
18-18: Looks good, class!The import statement for the
@Disabledannotation has been added correctly. This annotation will come in handy for disabling test classes or methods.
…ection (appsmithorg#36451) ## Description Ref: https://theappsmith.slack.com/archives/C02GAUE9P5H/p1726806965773469 ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!WARNING] > Tests have not run on the HEAD eab6cef yet > <hr>Fri, 20 Sep 2024 10:21:35 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Disabled test execution for the `MssqlGetDBSchemaTest` class to improve test suite performance. - Disabled test execution for the `MssqlPluginTest` class to streamline testing processes. - Disabled test execution for the `RedisPluginTest` class to enhance overall testing efficiency. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 participants
Description
Ref: https://theappsmith.slack.com/archives/C02GAUE9P5H/p1726806965773469
🔍 Cypress test results
Warning
Tests have not run on the HEAD eab6cef yet
Fri, 20 Sep 2024 10:21:35 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
MssqlGetDBSchemaTestclass to improve test suite performance.MssqlPluginTestclass to streamline testing processes.RedisPluginTestclass to enhance overall testing efficiency.