Skip to content

Attempting to fix the type check error #925

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

Merged
kohsuke merged 11 commits into from
Nov 7, 2024
+64 −71
Merged

Attempting to fix the type check error #925

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
c0d7078
Attempting to fix the type check error
kohsuke Nov 5, 2024
2df6616
Attempting to track down the flakiness
kohsuke Nov 5, 2024
570eb9e
add comments
Konboi Nov 6, 2024
147330a
refactor tests
Konboi Nov 6, 2024
266e3c1
for debug
Konboi Nov 6, 2024
1b821d4
add prefix to avoid conflict
Konboi Nov 6, 2024
20dd6aa
Don't know the cause, but in the Python 3.10 environment, we need to …
Konboi Nov 6, 2024
c92a645
Removing this as instructed by Konboi
kohsuke Nov 6, 2024
e7ded9a
Attempting to fix the type check error
kohsuke Nov 5, 2024
a12625c
Attempting to track down the flakiness
kohsuke Nov 5, 2024
6499494
Merge branch 'fix-flaky-split-test' into type-error-fix
kohsuke 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
6 changes: 3 additions & 3 deletions launchable/utils/sax.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
from typing import Callable, Dict, List, Optional
from xml.sax import make_parser
from xml.sax.handler import ContentHandler

from xml.sax.xmlreader import AttributesImpl
import click


Expand All @@ -22,7 +22,7 @@ class Element:
# tags captured at this context
# tags: Dict[str,object]

def __init__(self, parent: 'Element', name: str, attrs):
def __init__(self, parent: 'Element', name: str, attrs: AttributesImpl):
self.name = name
self.attrs = attrs
self.parent = parent
Expand Down Expand Up @@ -52,7 +52,7 @@ def __init__(self, element: str, attr: str, var: str):
self.attr = attr
self.var = var

def matches(self, e: Element) -> str:
def matches(self, e: Element) -> Optional[str]:
return e.attrs.get(
self.attr) if self.element == e.name or self.element == "*" else None

Expand Down
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

Add error handling and improve type safety.

The method should handle potential file-related errors and ensure consistent encoding.

Consider this improved implementation:

-    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 | Path, content: str, encoding: str = 'utf-8') -> None:
+        """Assert that a file's contents match the expected content.
+        
+        Args:
+            file_path: Path to the file to check
+            content: Expected content
+            encoding: File encoding (default: utf-8)
+            
+        Raises:
+            AssertionError: If the file doesn't exist or content doesn't match
+        """
+        path = Path(file_path)
+        self.assertTrue(path.exists(), f"File not found: {path}")
+        try:
+            with open(path, encoding=encoding) as f:
+                self.assertEqual(f.read(), content)
+        except IOError as e:
+            self.fail(f"Failed to read file {path}: {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 | Path, content: str, encoding: str = 'utf-8') -> None:
"""Assert that a file's contents match the expected content.
Args:
file_path: Path to the file to check
content: Expected content
encoding: File encoding (default: utf-8)
Raises:
AssertionError: If the file doesn't exist or content doesn't match
"""
path = Path(file_path)
self.assertTrue(path.exists(), f"File not found: {path}")
try:
with open(path, encoding=encoding) as f:
self.assertEqual(f.read(), content)
except IOError as e:
self.fail(f"Failed to read file {path}: {e}")


def assert_file_exists(self, file_path: str, exists: bool = True):
self.assertEqual(Path(file_path).exists(), exists)

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
122 changes: 54 additions & 68 deletions tests/commands/test_split_subset.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,75 +146,65 @@ def test_split_by_group_names(self):
]
}

responses.replace(
responses.POST,
"{base_url}/intake/organizations/{organization}/workspaces/{workspace}/subset/{subset_id}/split-by-groups".format(
base_url=get_base_url(),
organization=self.organization,
workspace=self.workspace,
subset_id=self.subsetting_id,
),
json=mock_json_response,
status=200
)
"""
Note(Konboi):
Don't know the cause, but in the Python 3.10 environment,
the settings configured with responses.replace disappear on the second call.
see: https://github.com/launchableinc/cli/actions/runs/11697720998/job/32576899978#step:10:88
So, to call it each time, `replace_response` was defined.
"""
def replace_response():
responses.replace(
responses.POST,
"{base_url}/intake/organizations/{organization}/workspaces/{workspace}/subset/{subset_id}/split-by-groups".format(
base_url=get_base_url(),
organization=self.organization,
workspace=self.workspace,
subset_id=self.subsetting_id,
),
json=mock_json_response,
status=200
)

with tempfile.TemporaryDirectory() as tmpdir:
replace_response()
result = self.cli("split-subset", "--subset-id", "subset/{}".format(self.subsetting_id),
"--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.assertFalse(os.path.exists("{}/rest-e2e.txt".format(tmpdir)))
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("{}/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.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:
replace_response()
result = self.cli("split-subset", "--subset-id", "subset/{}".format(self.subsetting_id),
"--split-by-groups-with-rest", "--split-by-groups-output-dir", tmpdir, "file")
"--split-by-groups-with-rest", "--split-by-groups-output-dir", tmpdir, "file", mix_stderr=False)

self.assert_success(result)
# this test is flaky -- let's see what's going on
print(result.stdout)
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 +263,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)