Skip to content

fix: silently skip duplicate TestFiles.add() instead of raising#2144

Merged
aseembits93 merged 8 commits intomainfrom
fix/test-files-silent-dedup
May 8, 2026
Merged

fix: silently skip duplicate TestFiles.add() instead of raising#2144
aseembits93 merged 8 commits intomainfrom
fix/test-files-silent-dedup

Conversation

@KRRT7
Copy link
Copy Markdown
Collaborator

@KRRT7 KRRT7 commented May 7, 2026

Summary

Changes TestFiles.add() duplicate handling from raising ValueError to silently skipping (first-write-wins). Fixes Java e2e test failures where test discovery adds the same instrumented_behavior_file_path with different metadata fields.

Stack

Part of #2132 linear stack. Replaces merged-by-accident #2133.

Changes

  • codeflash/models/models.py: Use set for O(1) membership check, silently return on duplicate path
  • tests/test_test_files_add.py: Tests for dedup behavior

Test plan

  • test_add_duplicate_is_noop verifies silent skip
  • test_add_unique_test_file passes
  • test_add_many_files_performance passes

@KRRT7 KRRT7 force-pushed the fix/test-files-silent-dedup branch from 46f63d2 to 72b75d6 Compare May 7, 2026 17:28
@KRRT7 KRRT7 force-pushed the cf-fix-concurrency-measurement branch from 51fb98a to 9399561 Compare May 7, 2026 17:40
@KRRT7 KRRT7 force-pushed the fix/test-files-silent-dedup branch from 72b75d6 to 5208de8 Compare May 7, 2026 17:42
Base automatically changed from cf-fix-concurrency-measurement to main May 7, 2026 22:26
@KRRT7 KRRT7 requested a review from aseembits93 May 7, 2026 22:37
KRRT7 added 5 commits May 7, 2026 17:46
The previous implementation used `if test_file not in self.test_files`
which performs O(n) equality comparison across all TestFile objects.
Replace with a `_seen_paths` set keyed on `instrumented_behavior_file_path`
for O(1) deduplication lookups.
Java test discovery adds the same instrumented_behavior_file_path with
different fields. The old Pydantic __eq__ treated them as different;
the set-based dedup correctly identifies them as the same test file.

Since get_test_type_by_instrumented_file_path() returns on first path
match anyway, duplicates by path are dead weight. Silent skip (first
write wins) is both correct and O(1).
- Remove .as_posix() from --junitxml and --rcfile subprocess arguments;
  use native path format (str(path)) which works on all platforms.

- Replace get_cross_platform_subprocess_run_args() in
  execute_test_subprocess with plain subprocess.run(capture_output=True,
  close_fds=False). The Windows-specific CREATE_NEW_PROCESS_GROUP and
  stdin=DEVNULL flags caused pytest to sporadically fail to write its
  junitxml output file, resulting in flaky test_pickle_patcher failures.
  Matches the working implementation in codeflash-python.

- Fix 10 pre-existing mypy errors in support.py (bare generics,
  re-export issues, missing attribute guards).
When the test subprocess exits non-zero and produces no JUnit XML,
log the return code and stdout/stderr at WARNING level so the root
cause is visible in CI logs. Previously this was a generic "No test
results found" message that made Windows CI flakes impossible to
diagnose.

Also fixes pre-existing mypy strict errors in parse_xml.py:
- Add return type to _parse_func
- Type CompletedProcess[str] (subprocess uses text=True)
- Parameterize generic types (tuple, re.Match)
- Remove dead .decode() branches (stdout is already str)
@KRRT7 KRRT7 force-pushed the fix/test-files-silent-dedup branch from c58d349 to 50f5070 Compare May 7, 2026 22:47
@KRRT7 KRRT7 requested a review from misrasaurabh1 as a code owner May 7, 2026 22:47
@KRRT7 KRRT7 changed the base branch from main to fix/mypy-strict-errors May 7, 2026 22:49
@KRRT7 KRRT7 marked this pull request as draft May 7, 2026 22:53
Base automatically changed from fix/mypy-strict-errors to main May 8, 2026 00:02
@KRRT7 KRRT7 marked this pull request as ready for review May 8, 2026 00:08
fix: use native path format for pytest subprocess args on Windows
with custom_addopts():
run_args = get_cross_platform_subprocess_run_args(
cwd=cwd, env=env, timeout=timeout, check=False, text=True, capture_output=True
return subprocess.run(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

it will work on windows? @KRRT7


class TestFiles(BaseModel):
test_files: list[TestFile]
_seen_paths: set[Path] = PrivateAttr(default_factory=set)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why privateattr @KRRT7 ?

aseembits93
aseembits93 previously approved these changes May 8, 2026
fix: surface subprocess errors when pytest XML is missing
@aseembits93 aseembits93 enabled auto-merge May 8, 2026 00:16
@aseembits93 aseembits93 merged commit cafcd7f into main May 8, 2026
32 of 33 checks passed
@aseembits93 aseembits93 deleted the fix/test-files-silent-dedup branch May 8, 2026 00:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants