refactor(e2e): replace bespoke result reporter with standard JUnit report - #33758
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
|
Greptile SummaryThis PR replaces the hand-rolled
Confidence Score: 5/5Safe to merge — changes are confined to the e2e test harness, add no new dependencies, and touch no production or proxy code. The refactor deletes ~150 lines of hand-rolled serialization and replaces them with a 59-line module that delegates entirely to pytest's built-in JUnit reporter. All custom logic (package normalization, covers deduplication, idempotency guard) is unit-tested, and the end-to-end subprocess test verifies all four outcome paths. No production paths are touched. No files require special attention.
|
| Filename | Overview |
|---|---|
| tests/e2e/junit_properties.py | New module replacing the bespoke logfmt emitter; provides idempotent attach_result_properties, clean package normalization, and order-preserving covers deduplication via dict.fromkeys — all correct. |
| tests/e2e/conftest.py | Replaced pytest_runtest_makereport with pytest_collection_modifyitems; delegates to attach_result_properties (idempotent). Unused Generator import and e2e_result_reporter imports cleanly removed. |
| tests/e2e/e2e_result_reporter.py | Deleted; hand-rolled logfmt serialization, outcome mapping, and node-id parsing all superseded by the standard JUnit reporter plus junit_properties.py. |
| tests/e2e/test_junit_properties.py | Comprehensive test file: unit tests for package_from_nodeid and dedupe_covers, plus subprocess end-to-end checks for pass/fail/skip/setup-error outcomes and idempotency; all four previously flagged gaps are now covered. |
| tests/e2e/grafana/status_history_panels.md | Updated from logfmt-line artifact to JUnit XML artifact; documents property schema, outcome mapping, and the required infra-side XML-to-logfmt converter; existing LogQL queries left intact for when the converter ships. |
Reviews (2): Last reviewed commit: "refactor(e2e): replace bespoke result re..." | Re-trigger Greptile
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…port tests/e2e/e2e_result_reporter.py hand-rolled a per-test logfmt emitter that reimplemented outcome mapping, logfmt escaping, and node-id parsing to print one E2E_RESULT line per finished test. Outcome, duration, and node id are all things a standard pytest reporter already produces, so the only genuinely custom data is the covers marker ids and the normalized package label Delete the module and emit a standard pytest JUnit XML report (--junitxml) instead, carrying the two custom signals as user_properties (JUnit <property> entries) attached at collection time in pytest_collection_modifyitems, so they land on every test on every outcome including skips and setup errors. The small package/covers extraction lives in junit_properties.py and is unit tested plus checked end to end against a real JUnit artifact in test_junit_properties.py Shipping the JUnit report to Loki is a thin infra-side transform, documented in grafana/status_history_panels.md
1b2086e to
dadf0fb
Compare
|
Addressed both findings in dadf0fb:
@greptileai please review the current head dadf0fb |
…ties e2e test Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
71e0251
into
litellm_internal_staging
Relevant issues
Linear ticket
Resolves LIT-4552
Pre-Submission checklist
Screenshots / Proof of Fix
This is harness-internal reporting plumbing with no LLM or proxy behavior change, so there is no before/after model call; the proof is that a real pytest run now produces a standard JUnit report carrying the two custom signals per test on every outcome (pass, fail, skip, setup error). Captured at
dadf0fb131:Every
<testcase>carriespackageandcovers<property>entries on all four outcomes: pass is a bare<testcase>, fail carries a standard<failure>, skip carries<skipped>, and a fixture setup error carries<error>; the doubledcoversmarker is deduped to one id, and a test with no marker gets an emptycoversType
🧹 Refactoring
Changes
tests/e2e/e2e_result_reporter.pyhand-rolled a per-test logfmt emitter: it reimplemented outcome mapping, logfmt escaping, and node-id parsing to print oneE2E_RESULTline per finished test, which a downstream Loki pipeline scraped from pod stdout. Outcome, duration, and node id are all things a standard pytest reporter already produces, so the only genuinely custom data was thecoversmarker ids and the normalizedpackagelabelThis deletes the bespoke module and switches to pytest's built-in JUnit XML report (
--junitxml). The two custom signals ride along asuser_properties, which JUnit records as<property>entries per testcase. They are attached at collection time inconftest.py::pytest_collection_modifyitems, so they land on every test on every outcome, including skips and setup errors, matching what the oldpytest_runtest_makereporthook did. The small package normalization and covers extraction live injunit_properties.py; nothing hand-rolls serialization, outcome mapping, or escaping anymoreJUnit XML is built into pytest, so this adds no dependency and no
uv.lockchange;pytest-reportlog(JSON-lines) would be the nicer Loki fit but needs a new pinned dist, whichuv sync --frozenin CI rejects without a lock regen, so it is a clean follow-up rather than part of this changeThe
E2E_RESULTlogfmt line was consumed only by an external Loki/Grafana pipeline that scraped it from pod stdout; nothing in this repo reads it, and no CI job runstests/e2e. JUnit XML is not line-based, so shipping it to Loki is a thin converter in the e2e job (walk the XML, print one logfmt line per<testcase>to stdout for the existing scrape); that transform is infra-side and lives outside this repoFinal Attestation
Link to Devin session: https://app.devin.ai/sessions/50af8d97faf642cf836f39016cd70667
Requested by: @yassin-berriai