-
Notifications
You must be signed in to change notification settings - Fork 8
Add tests for abstract classes #440
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
Conversation
WalkthroughTwo new test modules have been added. The first tests the abstract methods of Changes
Sequence Diagram(s)sequenceDiagram
participant TestCase
participant AbstractClass
TestCase->>AbstractClass: Patch __abstractmethods__ to empty set
TestCase->>AbstractClass: Instantiate class
TestCase->>AbstractClass: Call abstract method
AbstractClass-->>TestCase: Return None
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
CodeRabbit Configuration File (
|
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/test_abstract.py(1 hunks)tests/test_scheduler_commands.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/test_abstract.py (1)
pysqa/base/abstract.py (1)
QueueAdapterAbstractClass(8-80)
🪛 Ruff (0.12.2)
tests/test_scheduler_commands.py
23-23: Redefinition of unused test_submit_job_command from line 19
(F811)
27-27: Redefinition of unused test_submit_job_command from line 23
(F811)
🔇 Additional comments (4)
tests/test_abstract.py (3)
1-4: LGTM!Clean and appropriate imports for testing abstract classes.
6-6: LGTM!Standard unittest test class declaration.
7-33: Excellent approach to testing abstract class methods!The use of
@patch.multipleto clear__abstractmethods__is a clever way to enable instantiation of abstract classes for testing. Each test method properly verifies that the abstract methods returnNoneby default, which aligns with the abstract class implementation where methods contain onlypassstatements.The test coverage is comprehensive, covering all abstract methods from
QueueAdapterAbstractClasswith appropriate parameters.tests/test_scheduler_commands.py (1)
2-2: LGTM!Appropriate import addition for the new test methods.
| class TestAbstractSchedulerCommands(unittest.TestCase): | ||
| @patch.multiple(SchedulerCommands, __abstractmethods__=set()) | ||
| def test_submit_job_command(self): | ||
| self.assertIsNone(SchedulerCommands().submit_job_command) | ||
|
|
||
| @patch.multiple(SchedulerCommands, __abstractmethods__=set()) | ||
| def test_submit_job_command(self): | ||
| self.assertIsNone(SchedulerCommands().delete_job_command) | ||
|
|
||
| @patch.multiple(SchedulerCommands, __abstractmethods__=set()) | ||
| def test_submit_job_command(self): | ||
| self.assertIsNone(SchedulerCommands().get_queue_status_command) | ||
|
|
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.
Fix duplicate method names and incorrect method testing.
There are two critical issues with this test class:
- Duplicate method names: All three test methods are named
test_submit_job_command, but they should have unique names reflecting what they test. - Testing method references instead of calling methods: The assertions test method references (e.g.,
.submit_job_command) instead of calling the methods (e.g.,.submit_job_command()).
Apply this diff to fix both issues:
class TestAbstractSchedulerCommands(unittest.TestCase):
@patch.multiple(SchedulerCommands, __abstractmethods__=set())
def test_submit_job_command(self):
- self.assertIsNone(SchedulerCommands().submit_job_command)
+ self.assertIsNone(SchedulerCommands().submit_job_command())
@patch.multiple(SchedulerCommands, __abstractmethods__=set())
- def test_submit_job_command(self):
- self.assertIsNone(SchedulerCommands().delete_job_command)
+ def test_delete_job_command(self):
+ self.assertIsNone(SchedulerCommands().delete_job_command())
@patch.multiple(SchedulerCommands, __abstractmethods__=set())
- def test_submit_job_command(self):
- self.assertIsNone(SchedulerCommands().get_queue_status_command)
+ def test_get_queue_status_command(self):
+ self.assertIsNone(SchedulerCommands().get_queue_status_command())📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| class TestAbstractSchedulerCommands(unittest.TestCase): | |
| @patch.multiple(SchedulerCommands, __abstractmethods__=set()) | |
| def test_submit_job_command(self): | |
| self.assertIsNone(SchedulerCommands().submit_job_command) | |
| @patch.multiple(SchedulerCommands, __abstractmethods__=set()) | |
| def test_submit_job_command(self): | |
| self.assertIsNone(SchedulerCommands().delete_job_command) | |
| @patch.multiple(SchedulerCommands, __abstractmethods__=set()) | |
| def test_submit_job_command(self): | |
| self.assertIsNone(SchedulerCommands().get_queue_status_command) | |
| class TestAbstractSchedulerCommands(unittest.TestCase): | |
| @patch.multiple(SchedulerCommands, __abstractmethods__=set()) | |
| def test_submit_job_command(self): | |
| self.assertIsNone(SchedulerCommands().submit_job_command()) | |
| @patch.multiple(SchedulerCommands, __abstractmethods__=set()) | |
| def test_delete_job_command(self): | |
| self.assertIsNone(SchedulerCommands().delete_job_command()) | |
| @patch.multiple(SchedulerCommands, __abstractmethods__=set()) | |
| def test_get_queue_status_command(self): | |
| self.assertIsNone(SchedulerCommands().get_queue_status_command()) |
🧰 Tools
🪛 Ruff (0.12.2)
23-23: Redefinition of unused test_submit_job_command from line 19
(F811)
27-27: Redefinition of unused test_submit_job_command from line 23
(F811)
🤖 Prompt for AI Agents
In tests/test_scheduler_commands.py lines 17 to 29, rename the three test
methods to have unique names that reflect the method they test, such as
test_submit_job_command, test_delete_job_command, and
test_get_queue_status_command. Also, update the assertions to call the methods
(e.g., submit_job_command()) instead of referencing them, so use
self.assertIsNone(SchedulerCommands().submit_job_command()) and similarly for
the other methods.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #440 +/- ##
==========================================
+ Coverage 84.47% 85.30% +0.82%
==========================================
Files 17 17
Lines 966 966
==========================================
+ Hits 816 824 +8
+ Misses 150 142 -8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Summary by CodeRabbit