test(workflow-execution-service): extend ResultExportService unit test coverage#6902
Conversation
Automated Reviewer SuggestionsBased on the
|
There was a problem hiding this comment.
Pull request overview
Expands unit test coverage for ResultExportService’s export/encoding writer methods (CSV, Arrow, HTML, Parquet zip, single-cell data) by invoking private helpers via ScalaTest’s PrivateMethodTester and using an in-memory VirtualDocument fake to avoid external storage dependencies.
Changes:
- Adds an in-spec
VirtualDocument[Tuple]fake plus tracking input/output streams to test writer behavior in-process. - Introduces new tests covering CSV header behaviors, chunked CSV streaming, cell extraction/validation, field-to-bytes conversion, HTML export, Parquet-zip copy/close semantics, Arrow round-trip, and
NonClosingOutputStream.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
| config | throughput | MB/s | latency | max Δ latest / 7d | |
|---|---|---|---|---|---|
| 🔴 | bs=10 sw=10 sl=64 | 365 | 0.223 | 26,467/36,419/36,419 us | 🔴 +8.7% / 🔴 +119.5% |
| ⚪ | bs=100 sw=10 sl=64 | 795 | 0.485 | 125,728/151,800/151,800 us | ⚪ within ±5% / 🔴 +39.1% |
| ⚪ | bs=1000 sw=10 sl=64 | 896 | 0.547 | 1,122,018/1,142,495/1,142,495 us | ⚪ within ±5% / 🔴 +11.9% |
Baseline details
Latest main 4af3a85 from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 365 tuples/sec | 397 tuples/sec | 754.55 tuples/sec | -8.1% | -51.6% |
| bs=10 sw=10 sl=64 | MB/s | 0.223 MB/s | 0.242 MB/s | 0.461 MB/s | -7.9% | -51.6% |
| bs=10 sw=10 sl=64 | p50 | 26,467 us | 24,479 us | 12,816 us | +8.1% | +106.5% |
| bs=10 sw=10 sl=64 | p95 | 36,419 us | 33,500 us | 16,594 us | +8.7% | +119.5% |
| bs=10 sw=10 sl=64 | p99 | 36,419 us | 33,500 us | 19,806 us | +8.7% | +83.9% |
| bs=100 sw=10 sl=64 | throughput | 795 tuples/sec | 823 tuples/sec | 969.38 tuples/sec | -3.4% | -18.0% |
| bs=100 sw=10 sl=64 | MB/s | 0.485 MB/s | 0.502 MB/s | 0.592 MB/s | -3.4% | -18.0% |
| bs=100 sw=10 sl=64 | p50 | 125,728 us | 119,975 us | 103,584 us | +4.8% | +21.4% |
| bs=100 sw=10 sl=64 | p95 | 151,800 us | 150,180 us | 109,097 us | +1.1% | +39.1% |
| bs=100 sw=10 sl=64 | p99 | 151,800 us | 150,180 us | 117,304 us | +1.1% | +29.4% |
| bs=1000 sw=10 sl=64 | throughput | 896 tuples/sec | 911 tuples/sec | 1,004 tuples/sec | -1.6% | -10.7% |
| bs=1000 sw=10 sl=64 | MB/s | 0.547 MB/s | 0.556 MB/s | 0.613 MB/s | -1.6% | -10.7% |
| bs=1000 sw=10 sl=64 | p50 | 1,122,018 us | 1,097,333 us | 1,002,357 us | +2.2% | +11.9% |
| bs=1000 sw=10 sl=64 | p95 | 1,142,495 us | 1,151,730 us | 1,046,463 us | -0.8% | +9.2% |
| bs=1000 sw=10 sl=64 | p99 | 1,142,495 us | 1,151,730 us | 1,073,661 us | -0.8% | +6.4% |
Raw CSV
config_idx,batch_size,schema_width,string_len,num_batches,total_ms,total_tuples,total_bytes,tuples_per_sec,mb_per_sec,lat_p50_us,lat_p95_us,lat_p99_us
0,10,10,64,20,547.88,200,128000,365,0.223,26466.77,36418.75,36418.75
1,100,10,64,20,2517.26,2000,1280000,795,0.485,125728.42,151799.70,151799.70
2,1000,10,64,20,22323.78,20000,12800000,896,0.547,1122017.85,1142494.95,1142494.95
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6902 +/- ##
============================================
+ Coverage 78.39% 78.52% +0.13%
- Complexity 3723 3750 +27
============================================
Files 1161 1161
Lines 46066 46066
Branches 5107 5107
============================================
+ Hits 36112 36173 +61
+ Misses 8343 8281 -62
- Partials 1611 1612 +1
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…t coverage Cover the private export-encoding writers (streamDocumentAsCSV/Arrow/HTML/ ParquetZip, streamCellData, convertFieldToBytes) and NonClosingOutputStream via PrivateMethodTester and an in-spec fake VirtualDocument, so no Iceberg, MinIO or DB is involved.
409fa2c to
18adb51
Compare
What changes were proposed in this PR?
ResultExportServicesits at ~6% line coverage: the existing spec only coversparseOperatorsandvalidateExportRequest, leaving the whole export-encoding layer untested. ThestreamDocumentAs*writers look storage-bound but are not — each takes aVirtualDocument[Tuple]parameter, so a small in-spec fake document makes them testable against aByteArrayOutputStream, with no Iceberg, MinIO or DB.This extends
ResultExportServiceSpecwith 19 tests that reach the private writers throughPrivateMethodTester:streamDocumentAsCSV(both early returns, inferred vs supplied headers, and a document larger thanConstants.CHUNK_SIZEto exercise the buffered chunk loop),streamCellData(rowIndex/columnIndex validation messages, an unretrievable row, and the happy-path bytes),convertFieldToBytes(all three branches),streamDocumentAsHTML,streamDocumentAsParquetZip(verbatim copy plus source-stream close, and error propagation),streamDocumentAsArrow(round-trips the tuples back throughArrowFileReader), andNonClosingOutputStream.Two existing behaviors are pinned as-is rather than changed: with supplied headers the first row is not consumed by header inference, so it still appears in the body (the asymmetry against the
Nonebranch), andstreamDocumentAsHTMLthrowsNoSuchElementExceptionon an empty document because.headis unguarded.getOperatorDocument,saveToDatasets,saveStreamToDatasetandgenerateFileNamestay uncovered — they needDocumentFactory.openDocument, a live file-service endpoint, or embedLocalDateTime.now().Any related issues, documentation, discussions?
Closes #6898. Extends the spec added by #6705.
How was this PR tested?
sbt "WorkflowExecutionService/testOnly *ResultExportServiceSpec"passes with 24 tests (5 existing, 19 new);Test/scalafmtCheckandTest/scalafix --checkare clean. Verified the failure path by inverting the supplied-headers assertion and confirming the suite went red and exited non-zero. Every test runs in-process against an in-spec fake document, so no external service is involved.Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Fable 5)