Skip to content

Refactor split test #927

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.

Sign up for GitHub

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

Closed
Konboi wants to merge 2 commits into from
Closed

Refactor split test #927

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
83586b1
add comments
Konboi Nov 6, 2024
903662f
refactor tests
Konboi Nov 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions tests/cli_test_case.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,13 @@ def assert_success(self, result: click.testing.Result):
def assert_exit_code(self, result: click.testing.Result, expected: int):
self.assertEqual(result.exit_code, expected, result.stdout)

def assert_contents(self, file_path: str, content: str):
with open(file_path) as f:
self.assertEqual(f.read(), content)
Comment on lines +219 to +221
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
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)}")


def assert_file_exists(self, file_path: str, exists: bool = True):
self.assertEqual(Path(file_path).exists(), exists)
Comment on lines +223 to +224
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
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 find_request(self, url_suffix: str, n: int = 0):
'''Find the first (or n-th, if specified) request that matches the given suffix'''
for call in responses.calls:
Expand Down
82 changes: 32 additions & 50 deletions tests/commands/test_split_subset.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,24 +163,19 @@ def test_split_by_group_names(self):
"--split-by-groups", "--split-by-groups-output-dir", tmpdir, "file")

self.assert_success(result)
with open("{}/subset-e2e.txt".format(tmpdir)) as f:
self.assertEqual(f.read(), "e2e-aaa.py\ne2e-bbb.py")
self.assert_contents("{}/subset-e2e.txt".format(tmpdir), "e2e-aaa.py\ne2e-bbb.py")
self.assert_contents("{}/subset-{}.txt".format(tmpdir, SPLIT_BY_GROUPS_NO_GROUP_NAME), "aaa.py\nbbb.py")
# check the group file
self.assert_contents("{}/{}".format(tmpdir, SPLIT_BY_GROUP_SUBSET_GROUPS_FILE_NAME), "e2e")

self.assertFalse(os.path.exists("{}/rest-e2e.txt".format(tmpdir)))
# server doesn't return subset of unit-test
self.assert_file_exists("{}/subset-unit-test.txt".format(tmpdir), False)

self.assertFalse(os.path.exists("{}/subset-unit-test.txt".format(tmpdir)))

self.assertFalse(os.path.exists("{}/rest-unit-test.txt".format(tmpdir)))

with open("{}/subset-{}.txt".format(tmpdir, SPLIT_BY_GROUPS_NO_GROUP_NAME)) as f:
self.assertEqual(f.read(), "aaa.py\nbbb.py")

self.assertFalse(os.path.exists("{}/rest-{}.txt".format(tmpdir, SPLIT_BY_GROUPS_NO_GROUP_NAME)))

with open("{}/{}".format(tmpdir, SPLIT_BY_GROUP_SUBSET_GROUPS_FILE_NAME)) as f:
self.assertEqual(f.read(), "e2e")

self.assertFalse(os.path.exists("{}/{}".format(tmpdir, SPLIT_BY_GROUP_REST_GROUPS_FILE_NAME)))
# doesn't set the --rest option
self.assert_file_exists("{}/rest-e2e.txt".format(tmpdir), False)
self.assert_file_exists("{}/rest-unit-test.txt".format(tmpdir), False)
self.assert_file_exists("{}/rest-{}.txt".format(tmpdir, SPLIT_BY_GROUPS_NO_GROUP_NAME), False)
self.assert_file_exists("{}/{}".format(tmpdir, SPLIT_BY_GROUP_REST_GROUPS_FILE_NAME), False)

# with rest option
with tempfile.TemporaryDirectory() as tmpdir:
Expand All @@ -193,28 +188,19 @@ def test_split_by_group_names(self):
for item in os.listdir(tmpdir):
print(item)

with open(os.path.join(tmpdir, "subset-e2e.txt")) as f:
self.assertEqual(f.read(), "e2e-aaa.py\ne2e-bbb.py")

with open(os.path.join(tmpdir, "rest-e2e.txt")) as f:
self.assertEqual(f.read(), "e2e-ccc.py\ne2e-ddd.py")
self.assert_contents("{}/subset-e2e.txt".format(tmpdir), "e2e-aaa.py\ne2e-bbb.py")
self.assert_contents("{}/rest-e2e.txt".format(tmpdir), "e2e-ccc.py\ne2e-ddd.py")

self.assertFalse(os.path.exists("{}/subset-unit-test.txt".format(tmpdir)))
# server doesn't return subset of unit-test
self.assert_file_exists("{}/subset-unit-test.txt".format(tmpdir), False)
self.assert_contents("{}/rest-unit-test.txt".format(tmpdir), "unit-test-111.py\nunit-test-222.py")

with open(os.path.join(tmpdir, "rest-unit-test.txt")) as f:
self.assertEqual(f.read(), "unit-test-111.py\nunit-test-222.py")
self.assert_contents("{}/subset-{}.txt".format(tmpdir, SPLIT_BY_GROUPS_NO_GROUP_NAME), "aaa.py\nbbb.py")
self.assert_contents("{}/rest-{}.txt".format(tmpdir, SPLIT_BY_GROUPS_NO_GROUP_NAME), "111.py\n222.py")

with open(os.path.join(tmpdir, "subset-{}.txt".format(SPLIT_BY_GROUPS_NO_GROUP_NAME))) as f:
self.assertEqual(f.read(), "aaa.py\nbbb.py")

with open(os.path.join(tmpdir, "rest-{}.txt".format(SPLIT_BY_GROUPS_NO_GROUP_NAME))) as f:
self.assertEqual(f.read(), "111.py\n222.py")

with open(os.path.join(tmpdir, SPLIT_BY_GROUP_SUBSET_GROUPS_FILE_NAME)) as f:
self.assertEqual(f.read(), "e2e")

with open(os.path.join(tmpdir, SPLIT_BY_GROUP_REST_GROUPS_FILE_NAME)) as f:
self.assertEqual(f.read(), "unit-test")
# check the group file
self.assert_contents("{}/{}".format(tmpdir, SPLIT_BY_GROUP_SUBSET_GROUPS_FILE_NAME), "e2e")
self.assert_contents("{}/{}".format(tmpdir, SPLIT_BY_GROUP_REST_GROUPS_FILE_NAME), "unit-test")

@responses.activate
@mock.patch.dict(os.environ, {"LAUNCHABLE_TOKEN": CliTestCase.launchable_token})
Expand Down Expand Up @@ -273,19 +259,15 @@ def test_split_by_group_names_output_exclusion_rules(self):
"--split-by-groups", "--split-by-groups-output-dir", tmpdir, '--output-exclusion-rules', "file")

self.assert_success(result)
with open("{}/subset-e2e.txt".format(tmpdir)) as f:
self.assertCountEqual(f.read().splitlines(), ["e2e-ccc.py", "e2e-ddd.py", ])

self.assertFalse(os.path.exists("{}/rest-e2e.txt".format(tmpdir)))

with open("{}/subset-unit-test.txt".format(tmpdir)) as f:
self.assertCountEqual(f.read().splitlines(), ["unit-test-111.py", "unit-test-222.py"])
self.assertFalse(os.path.exists("{}/rest-unit-test.txt".format(tmpdir)))

with open("{}/subset-{}.txt".format(tmpdir, SPLIT_BY_GROUPS_NO_GROUP_NAME)) as f:
self.assertCountEqual(f.read().splitlines(), ["111.py", "222.py"])
self.assertFalse(os.path.exists("{}/rest-{}.txt".format(tmpdir, SPLIT_BY_GROUPS_NO_GROUP_NAME)))

with open("{}/{}".format(tmpdir, SPLIT_BY_GROUP_SUBSET_GROUPS_FILE_NAME)) as f:
self.assertEqual(f.read(), "unit-test")
self.assertFalse(os.path.exists("{}/{}".format(tmpdir, SPLIT_BY_GROUP_REST_GROUPS_FILE_NAME)))
# --output-exclusion-rules is enabled, thus switched subset and rest
self.assert_contents("{}/subset-e2e.txt".format(tmpdir), "e2e-ccc.py\ne2e-ddd.py")
self.assert_contents("{}/subset-unit-test.txt".format(tmpdir), "unit-test-111.py\nunit-test-222.py")
self.assert_contents("{}/subset-{}.txt".format(tmpdir, SPLIT_BY_GROUPS_NO_GROUP_NAME), "111.py\n222.py")
self.assert_contents("{}/{}".format(tmpdir, SPLIT_BY_GROUP_SUBSET_GROUPS_FILE_NAME), "unit-test")

# doesn't set the --rest option
self.assert_file_exists("{}/rest-e2e.txt".format(tmpdir), False)
self.assert_file_exists("{}/rest-unit-test.txt".format(tmpdir), False)
self.assert_file_exists("{}/rest-{}.txt".format(tmpdir, SPLIT_BY_GROUPS_NO_GROUP_NAME), False)
self.assert_file_exists("{}/{}".format(tmpdir, SPLIT_BY_GROUP_REST_GROUPS_FILE_NAME), False)