Skip to content

Conversation

@aseembits93
Copy link
Contributor

@aseembits93 aseembits93 commented Oct 28, 2025

For codeflash-ai/datacompy#13
Before
Screenshot 2025-10-28 at 11 38 53
Now
Screenshot 2025-10-28 at 11 35 04

@github-actions
Copy link

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Possible Under-selection

Switching to function-only pytest node ids for replay tests drops class qualification and param parametrization stripping, which may miss tests with identical function names in different classes or skip parametrized variants.

    if file.test_type == TestType.REPLAY_TEST:
        # TODO: Does this work for unittest framework?
        test_files.extend(
            [str(file.benchmarking_file_path) + "::" + test.test_function for test in file.tests_in_file]
        )
    else:
        test_files.append(str(file.benchmarking_file_path))
test_files = list(set(test_files))  # remove multiple calls in the same test function
Behavior Change

Existing unit tests are no longer explicitly targeted; only replay tests are expanded. Confirm this matches the intent to "align selection" and that benchmark/LP still include intended existing tests.

    if file.test_type == TestType.REPLAY_TEST:
        # TODO: Does this work for unittest framework?
        test_files.extend(
            [str(file.benchmarking_file_path) + "::" + test.test_function for test in file.tests_in_file]
        )
    else:
        test_files.append(str(file.benchmarking_file_path))
test_files = list(set(test_files))  # remove multiple calls in the same test function
Framework Assumption

The TODO suggests uncertainty about unittest support; current logic assumes pytest-style node ids and may break when test_framework != pytest for replay selections.

if file.test_type == TestType.REPLAY_TEST:
    # TODO: Does this work for unittest framework?
    test_files.extend(
        [str(file.benchmarking_file_path) + "::" + test.test_function for test in file.tests_in_file]
    )

@github-actions
Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Handle empty test lists safely

Guard against empty tests_in_file to avoid generating path:: without a function,
which pytest will error on. Fallback to file-only selection when no functions are
listed.

codeflash/verification/test_runner.py [169-175]

-if file.test_type == TestType.REPLAY_TEST:
+if file.test_type == TestType.REPLAY_TEST and file.tests_in_file:
     # TODO: Does this work for unittest framework?
     test_files.extend(
-        [str(file.benchmarking_file_path) + "::" + test.test_function for test in file.tests_in_file]
+        [
+            str(file.benchmarking_file_path)
+            + "::"
+            + (test.test_class + "::" if getattr(test, "test_class", None) else "")
+            + test.test_function
+            for test in file.tests_in_file
+        ]
     )
 else:
     test_files.append(str(file.benchmarking_file_path))
Suggestion importance[1-10]: 7

__

Why: Guarding on file.tests_in_file prevents constructing invalid path:: entries and mirrors the previous behavior; the change is accurate and improves robustness, though not a critical bug fix.

Medium
Possible issue
Add class to node ids

Include the test class in node ids for methods inside classes. Omitting
test.test_class can cause pytest to miss or wrongly match tests when function names
collide across classes.

codeflash/verification/test_runner.py [172]

-[str(file.benchmarking_file_path) + "::" + test.test_function for test in file.tests_in_file]
+[
+    str(file.benchmarking_file_path)
+    + "::"
+    + (test.test_class + "::" if getattr(test, "test_class", None) else "")
+    + test.test_function
+    for test in file.tests_in_file
+]
Suggestion importance[1-10]: 6

__

Why: Omitting test_class can cause ambiguous node ids when functions share names across classes; adding it aligns with the removed old logic. This is contextually accurate and improves selection reliability.

Low
Correct parametrized test handling

Preserve parametrized test names when building pytest node ids. Dropping the
parameter portion (e.g., text in brackets) changes selection and can over/under-run
tests.

codeflash/verification/test_runner.py [172]

-[str(file.benchmarking_file_path) + "::" + test.test_function for test in file.tests_in_file]
+[
+    str(file.benchmarking_file_path)
+    + "::"
+    + (test.test_function if "[" not in test.test_function else test.test_function.split("[", 1)[0])
+    for test in file.tests_in_file
+]
Suggestion importance[1-10]: 3

__

Why: The line exists at 172, but the suggestion is internally inconsistent: it claims to preserve parametrization while the improved code actually strips parameters. Impact is minor and correctness is questionable.

Low

@aseembits93
Copy link
Contributor Author

you might notice that the generated tests are passing for behavior and failing for benchmark, that's a different issue, I'll create a separate PR for it on the backend. @misrasaurabh1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants