Refactor split test #927
Refactor split test #927
Conversation
WalkthroughThe changes introduce two new methods to the Changes
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
CodeRabbit Configuration File (
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
tests/commands/test_split_subset.py (2)
Line range hint
184-187: Remove debug print statements.These debug print statements were likely added for troubleshooting the flaky test and should be removed:
- # this test is flaky -- let's see what's going on - print(result.stdout) - for item in os.listdir(tmpdir): - print(item)
263-273: LGTM! Consider adding more test coverage.The test effectively verifies the basic behavior of
--output-exclusion-rules. However, consider adding test cases to verify:
- The actual exclusion rules logic (e.g., how tests are excluded)
- Edge cases where exclusion rules might interact with different group configurations
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
tests/cli_test_case.py(1 hunks)tests/commands/test_split_subset.py(3 hunks)
🔇 Additional comments (2)
tests/commands/test_split_subset.py (2)
166-178: LGTM! Improved test assertions.
The new assertion methods (assert_contents and assert_file_exists) make the test more readable and maintainable. The checks are well-organized:
- Verifying subset file contents
- Checking group file contents
- Explicitly verifying non-existence of files
191-203: LGTM! Well-structured test assertions.
The test cases are well-organized and comprehensive:
- Verifying both subset and rest file contents
- Checking for expected missing files
- Validating group file contents for both subset and rest
| def assert_file_exists(self, file_path: str, exists: bool = True): | ||
| self.assertEqual(Path(file_path).exists(), exists) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve documentation and error messages of assert_file_exists.
The method would benefit from better documentation and more descriptive assertion messages:
- def assert_file_exists(self, file_path: str, exists: bool = True):
- self.assertEqual(Path(file_path).exists(), exists)
+ def assert_file_exists(self, file_path: str, exists: bool = True) -> None:
+ """Assert that a file exists or does not exist.
+
+ Args:
+ file_path: Path to the file to check
+ exists: If True, assert the file exists. If False, assert it doesn't exist
+
+ Raises:
+ AssertionError: If the file existence doesn't match expectations
+ """
+ file_exists = Path(file_path).exists()
+ if exists:
+ self.assertTrue(file_exists, f"Expected file to exist: {file_path}")
+ else:
+ self.assertFalse(file_exists, f"Expected file to not exist: {file_path}")📝 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.
| def assert_file_exists(self, file_path: str, exists: bool = True): | |
| self.assertEqual(Path(file_path).exists(), exists) | |
| def assert_file_exists(self, file_path: str, exists: bool = True) -> None: | |
| """Assert that a file exists or does not exist. | |
| Args: | |
| file_path: Path to the file to check | |
| exists: If True, assert the file exists. If False, assert it doesn't exist | |
| Raises: | |
| AssertionError: If the file existence doesn't match expectations | |
| """ | |
| file_exists = Path(file_path).exists() | |
| if exists: | |
| self.assertTrue(file_exists, f"Expected file to exist: {file_path}") | |
| else: | |
| self.assertFalse(file_exists, f"Expected file to not exist: {file_path}") |
| def assert_contents(self, file_path: str, content: str): | ||
| with open(file_path) as f: | ||
| self.assertEqual(f.read(), content) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance robustness and documentation of assert_contents.
The method could benefit from improved error handling and documentation:
- def assert_contents(self, file_path: str, content: str):
- with open(file_path) as f:
- self.assertEqual(f.read(), content)
+ def assert_contents(self, file_path: str, content: str) -> None:
+ """Assert that the contents of a file match the expected content.
+
+ Args:
+ file_path: Path to the file to check
+ content: Expected content of the file
+
+ Raises:
+ AssertionError: If the file contents don't match
+ FileNotFoundError: If the file doesn't exist
+ IOError: If there are issues reading the file
+ """
+ try:
+ with open(file_path, encoding='utf-8') as f:
+ self.assertEqual(f.read(), content)
+ except FileNotFoundError:
+ self.fail(f"File not found: {file_path}")
+ except IOError as e:
+ self.fail(f"Failed to read file {file_path}: {str(e)}")📝 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.
| def assert_contents(self, file_path: str, content: str): | |
| with open(file_path) as f: | |
| self.assertEqual(f.read(), content) | |
| def assert_contents(self, file_path: str, content: str) -> None: | |
| """Assert that the contents of a file match the expected content. | |
| Args: | |
| file_path: Path to the file to check | |
| content: Expected content of the file | |
| Raises: | |
| AssertionError: If the file contents don't match | |
| FileNotFoundError: If the file doesn't exist | |
| IOError: If there are issues reading the file | |
| """ | |
| try: | |
| with open(file_path, encoding='utf-8') as f: | |
| self.assertEqual(f.read(), content) | |
| except FileNotFoundError: | |
| self.fail(f"File not found: {file_path}") | |
| except IOError as e: | |
| self.fail(f"Failed to read file {file_path}: {str(e)}") |
SSIA
Summary by CodeRabbit
New Features
Bug Fixes
SplitSubsetTestclass to streamline validation of outputs from thesplit-subsetcommand.Tests