rework new process discovery

[KRRT7]

User description

previously, test results were written after pytest.main() completed, but pytest doesn't always exit with a success code even when collection is successful

this fixes test collection for https://github.com/temporalio/sdk-python

---

PR Type

Bug fix, Enhancement

---

Description

• Write collection results during callback

• Remove reliance on pytest exit code

• Add safe file write via pathlib

• Simplify pytest invocation arguments

---

Diagram Walkthrough

flowchart LR
  start["pytest collection starts"] -- "collect items" --> hook["pytest_collection_finish hook"]
  hook -- "serialize items + rootdir" --> write["write (0, tests, rootdir) to pickle"]
  write -- "pytest exits (code ignored)" --> end["discovery completes"]

Loading

File Walkthrough

	Relevant files
Bug fix	pytest_new_process_discovery.py Persist collected tests during pytest hook                              codeflash/discovery/pytest_new_process_discovery.py Write pickle in pytest_collection_finish hook. Use Path for binary write and pickle highest protocol. Stop tracking/returning exitcode; remove post-main write. Adjust pytest args order; keep skip/benchmark logic. +25/-21

---

[github-actions]

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 Robustness Writing results inside the collection-finish hook assumes the hook always fires and that write succeeds; consider handling file I/O errors and ensuring atomicity or a fallback write on process exit. # Write results immediately since pytest.main() will exit after this callback, not always with a success code tests = parse_pytest_collection_results(collected_tests) with Path(pickle_path).open("wb") as f: import pickle pickle.dump((0, tests, pytest_rootdir), f, protocol=pickle.HIGHEST_PROTOCOL) Global State The callback now mutates and serializes using globals; ensure no race or re-entrancy issues when plugins modify items or when pytest runs with xdist or multiple sessions. global pytest_rootdir, collected_tests collected_tests.extend(session.items) pytest_rootdir = session.config.rootdir # Write results immediately since pytest.main() will exit after this callback, not always with a success code tests = parse_pytest_collection_results(collected_tests) with Path(pickle_path).open("wb") as f: import pickle pickle.dump((0, tests, pytest_rootdir), f, protocol=pickle.HIGHEST_PROTOCOL) Plugin Args The pytest.main invocation splits -p options; confirm combined string vs separate args behavior and ensure desired plugins are actually disabled in all pytest versions. pytest.main( [tests_root, "-p", "no:logging", "--collect-only", "-m", "not skip", "-p", "no:codeflash-benchmark"], plugins=[PytestCollectionPlugin()], )

[github-actions]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Persist accurate exit status Writing an exit code of 0 unconditionally can mask collection failures. Use session.config.exitstatus if available to persist the real exit code, defaulting to a non-zero on error. codeflash/discovery/pytest_new_process_discovery.py [30-40] class PytestCollectionPlugin: def pytest_collection_finish(self, session) -> None: global pytest_rootdir, collected_tests collected_tests.extend(session.items) pytest_rootdir = session.config.rootdir - # Write results immediately since pytest.main() will exit after this callback, not always with a success code tests = parse_pytest_collection_results(collected_tests) + exit_code = getattr(session.config, "exitstatus", 0) with Path(pickle_path).open("wb") as f: import pickle - pickle.dump((0, tests, pytest_rootdir), f, protocol=pickle.HIGHEST_PROTOCOL) + pickle.dump((exit_code, tests, pytest_rootdir), f, protocol=pickle.HIGHEST_PROTOCOL) Suggestion importance[1-10]: 8 __ Why: Unconditionally writing exit code 0 can mask collection failures; using session.config.exitstatus preserves correctness. This directly addresses a logical correctness issue with clear mapping to the code.	Medium
	Write failure results on exception If pytest.main raises, no results are written, leaving the parent process without data. In the exception path, write a pickle file with a non-zero exit code and empty tests to signal failure deterministically. codeflash/discovery/pytest_new_process_discovery.py [52-58] if __name__ == "__main__": import pytest try: pytest.main( [tests_root, "-p", "no:logging", "--collect-only", "-m", "not skip", "-p", "no:codeflash-benchmark"], plugins=[PytestCollectionPlugin()], ) except Exception as e: print(f"Failed to collect tests: {e!s}") + from pathlib import Path as _Path + import pickle as _pickle + with _Path(pickle_path).open("wb") as f: + _pickle.dump((1, [], None), f, protocol=_pickle.HIGHEST_PROTOCOL) Suggestion importance[1-10]: 7 __ Why: Ensuring a pickle is written on exceptions makes the parent process robust to failures; the change is accurate and useful, though not critical since normal flow already writes results.	Medium
	Safely access optional attributes Accessing test.cls without ensuring the attribute exists can raise an AttributeError for non-function items. Use getattr to safely read optional attributes and guard test.path similarly to avoid crashes with non-standard nodes. codeflash/discovery/pytest_new_process_discovery.py [15-26] def parse_pytest_collection_results(pytest_tests: list[Any]) -> list[dict[str, str]]: test_results = [] for test in pytest_tests: test_class = None - if test.cls: - test_class = test.parent.name + if getattr(test, "cls", None): + # Prefer class name if available; fallback to parent name if present + test_class = getattr(getattr(test, "cls", None), "__name__", None) or getattr(getattr(test, "parent", None), "name", None) + test_file = str(getattr(test, "path", "")) if getattr(test, "path", None) is not None else "" test_results.append({ - "test_file": str(test.path), - "test_class": test_class, - "test_function": test.name + "test_file": test_file, + "test_class": test_class, + "test_function": getattr(test, "name", "") }) return test_results Suggestion importance[1-10]: 6 __ Why: Using getattr to guard attribute access reduces risk of AttributeError during collection, and the improved_code reflects the intended safety change. Impact is moderate as current code may generally work but could break on atypical nodes.	Low

[aseembits93]

originally we're setting exitcode as -1 in the case of exception but here it's 1

[aseembits93]

I guess it was not correct to have it as -1 in the first place according to the docs -> https://docs.pytest.org/en/stable/reference/exit-codes.html

[aseembits93]

LGTM, a little bit skeptical about the validity of exit_code = getattr(session.config, "exitstatus", 0)