Skip to content

Add stdDevPop, stdDevSamp window operator and setWindowFields stage tests#693

Open
Cassie-2025 wants to merge 1 commit into
documentdb:mainfrom
Cassie-2025:window-stddev-tests
Open

Add stdDevPop, stdDevSamp window operator and setWindowFields stage tests#693
Cassie-2025 wants to merge 1 commit into
documentdb:mainfrom
Cassie-2025:window-stddev-tests

Conversation

@Cassie-2025

Copy link
Copy Markdown

Add compatibility tests for:

  • stdDevPop window operator (11 test files, 146 tests)
  • stdDevSamp window operator (11 test files, 146 tests)
  • setWindowFields stage error validation (5 test files, 33 tests)

Coverage includes: documents/range/time-range frame modes, partitioning, sort order, numeric precision, non-numeric handling, NaN/Infinity behavior, field paths, pipeline stage interactions, and frame validation errors.

All 325 tests verified passing.

…ests

Add compatibility tests for:
- stdDevPop window operator (11 test files, 146 tests)
- stdDevSamp window operator (11 test files, 146 tests)
- setWindowFields stage error validation (5 test files, 33 tests)

Coverage includes: documents/range/time-range frame modes, partitioning,
sort order, numeric precision, non-numeric handling, NaN/Infinity behavior,
field paths, pipeline stage interactions, and frame validation errors.

All 325 tests verified passing.

Signed-off-by: Cassie-2025 <Cassie-2025@users.noreply.github.com>
@Cassie-2025
Cassie-2025 requested a review from a team as a code owner July 17, 2026 03:41
@documentdb-triage-tool documentdb-triage-tool Bot added compatibility test Compatibility test related enhancement New feature or request labels Jul 17, 2026
@documentdb-triage-tool

Copy link
Copy Markdown

🤖 Auto-triaged by documentdb-triage-tool.

Applied: compatibility test, enhancement
Project fields suggested: Component test-coverage · Priority P2 · Effort XL · Status Needs Review
Confidence: 0.92 (mixed)

Reasoning

component from path globs (test-coverage); effort from diff stats (12099+0 LOC, 24 files); LLM: Adds 325 new compatibility tests across multiple files for stdDevPop/stdDevSamp window operators and setWindowFields stage, spanning documents/range/time-range frame modes and various edge cases.

If a label is wrong, remove it manually and ping @patty-chow so the rules can be tuned. The bot will not re-label items that already have component labels.

@drk-knght drk-knght left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

lgtm

@eerxuan eerxuan left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Summary

This is the first substantive window-operator PR — 325 tests across 24 files for $stdDevPop, $stdDevSamp, and $setWindowFields stage errors, where previously every window// folder and setWindowFields/ held only smoke tests. Because it sets the template for ~26 remaining window operators, the structural decisions matter more than the individual assertions. The stdDev
math itself is correct (population vs sample formulas, single-element pop=0 vs samp=null, deterministic sorts — no cross-contamination), but the PR bakes in significant duplication, omits the reviewer-requested TEST_COVERAGE.md spec, and deviates from the repo's near-universal test conventions.

Findings

  1. 🔴 Window-frame / partition / sort mechanics are tested per-operator and duplicated across both operators — they belong once at the stage level.
    documents_mode_frames, range_mode_frames, time_range_mode_frames, partitioning, sort, field_paths, and pipeline_stages (≈7 of 10 files per operator) test $setWindowFields stage behavior — how documents/range/time bounds select the frame, how partitionBy/sortBy work, pipeline composition. These are spec-consistent across all ~30 window operators, and the files are
    byte-parallel between stdDevPop and stdDevSamp (identical test-name sets: whole_partition, cumulative, sliding_centered, trailing, …; verified 0 error assertions — value-only). Per TEST_COVERAGE.md §18/§19 ("test once, representative operator") and REVIEW.md §8 ("duplicate coverage"), this matrix should live once under stages/setWindowFields/ driven by one
    representative operator. Left as-is, it becomes the precedent that every future window operator re-copies the full ~48-case frame matrix ×N. Keep per-operator only the genuine stdDev delta (numeric_precision, non_numeric_handling, special_floats + 1–2 wiring cases).

  2. 🟠 docs/testing/TEST_COVERAGE.md not updated, though this introduces the window-operator feature category.
    REVIEW.md §9 (line 85): "TEST_COVERAGE.md updated if the PR introduces coverage rules for a new feature category (e.g. date operators, window functions)" — window functions is the literal example, and the PR touches the doc 0 times. As the first window-operator PR, it must add a "Window Operator Coverage" section stating what's tested once at the $setWindowFields stage
    level (frame modes, partitionBy, sortBy, frame-validation errors — representative operator) vs per-operator (the operator's own math: formula, empty/single-element result, non-numeric handling, special floats, precision). Without this written rule, finding #1's duplication becomes the standing precedent.

  3. 🟠 Test-format deviation — 322 hand-written def test_* functions instead of the parametrized-dataclass pattern.
    TEST_FORMAT.md "Parametrized Tests" and REVIEW.md §4 mandate @DataClass(frozen=True) + pytest_params() for operators with many cases. This PR uses 0 pytest_params/parametrize and 322 standalone def test_* with a copy-pasted local _run_stdDev* helper — while 1603 existing files under tests/core/operator/ use pytest_params and none use this style. This is the lone
    deviation, and it's the template the next 26 window operators would copy. (Converting to parametrized dataclasses also collapses the 800-line files, so I'm not flagging length separately.)

  4. 🟠 Shared helper + corpus duplicated per file instead of a window/utils/ module.
    Every file re-defines its own _run_stdDevPop/_run_stdDevSamp pipeline-builder and BASIC_DOCS/numeric corpus (byte-identical across the two operators bar the operator name). REVIEW.md §5/§6 and TEST_FORMAT.md require shared helpers/data in utils/; sibling accumulators/utils (AccumulatorTestCase), stages/lookup/utils, etc. establish the convention. A window-pipeline
    change would need editing ~19 times.

  5. 🟡 Raw integer error codes in the stage-error files.
    REVIEW.md §4: "Error codes from framework.error_codes used (not raw integers)." The three setWindowFields_*_errors files make 29 assertFailureCode(result, ) calls (code 9 twenty times, despite FAILED_TO_PARSE_ERROR = 9 existing) with 0 error_codes imports — so REVIEW.md §8 ("test the right error code") can't be verified at a glance.

  6. 🟡 Inline float("nan")/("inf")/("-inf") literals instead of the existing constants.
    TEST_COVERAGE.md:613 "All test constants … defined in framework/test_constants.py. Import from there — do not duplicate values." FLOAT_NAN/FLOAT_INFINITY/FLOAT_NEGATIVE_INFINITY already exist; the special_floats/precision files hardcode them ~80×. (Note: the files do correctly use the established assertSuccessNaN idiom for NaN comparison — that part is compliant.)

  7. 🟡 No # Property [Name]: comments in any of the 24 files.
    REVIEW.md §4 / TEST_FORMAT.md require per-group # Property [...] comments; the diff has 0. As the category template, this drops the property-documentation convention wholesale.

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

Labels

compatibility test Compatibility test related enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants