Repository navigation
test: drop duplicate and stale tests, keep slow variants out of debug CI, run the DAG viewer tests in CI - #834
Merged
Merged
Conversation
…un the DAG viewer tests in CI Delete tests that repeat stronger ones, assertions that cannot fail, the unused cse::dag_node_count helper, and ignored tests whose premise no longer holds. Move the monthly Example 4 variants and the exhaustive-limit check out of the debug CI run, ratchet the metrics-observability corpus coverage, point ignore reasons at open issues, rename post-ASAP-named test helpers to the physical ASAP DAG terms, and run the DAG viewer's Python tests in CI.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
The test suite had grown slow, padded and stale in places:
monthly()variant (126 s, 67,228 candidates) and the exhaustive-limit devtool test (about 30 s) planned far more than they checked. The metrics-observability corpus test (41 s) only asserted that its own counters add up.What
Slow tests
POST_ASAP_MISSreport and now ratchets each corpus at today's counts: at least N queries with an ASAP candidate, and at most M Stage 1 errors. A regression in coverage now fails the test. It is renamedbenchmark_corpora_are_total_and_keep_their_coverage.once,at_restand Pattern B in CI. Themonthly()variant moves into two#[ignore = "slow: run in release by hand"]tests:…monthly_ingestion_time_upstream_is_ingestion_timeand…monthly_materialized_output_covers_its_consumers.exhaustive_default_candidate_limit_is_ten_thousandbecomes a unit test in thestage_pipelinebinary that asserts onMAX_COMBINATIONS.workload_above_the_candidate_limit_is_rejectedstays.Duplicate tests deleted. Each one was compared line by line with the test that stays.
promql_conformance::rate_range_lives_in_time_range_nodepromql_lowering::rate_has_time_range_child_not_window(identical),time_range::q13_rate(full DAG equality)promql_conformance::increase_range_lives_in_time_range_nodetime_range::q14_increase(full DAG equality)promql_conformance::sum_of_rate_is_two_levelspromql_lowering::sum_over_rate_keeps_both_levels(also checks the innerTimeRange)promql_conformance::sum_by_of_rate_groups_outer_levelpromql_lowering::sum_by_over_rate_groups_the_outer_sumpromql_conformance::histogram_quantile_over_sum_by_le_preserves_le_groupingpromql_lowering::histogram_quantile_over_sum_by_le_preserves_grouping(also checks q)promql_conformance::quantile_over_time_is_aggregate_over_time_rangepromql_lowering::quantile_over_time_is_time_range_aggregate,time_range::q17_quantile_over_timepromql_conformance::topk_over_count_is_heavy_hitterpromql_lowering::topk_over_count_is_heavy_hitter_topk(grouped, full chain),fractional_topk_k_truncates_toward_zero(ungrouped)promql_conformance::outer_group_key_absent_from_nested_aggregate_is_droppednested::q53_outer_group_key_absent_from_nested_aggregate(full DAG equality)promql_conformance::range_vector_selector_is_time_rangepromql_lowering::explicit_range_selector_is_a_range_time_range, which now also asserts the bare selector's 5-min range (the one check it lacked)awesome_prometheus_alerts::vector_literal_lowers_to_a_labelless_vectorpromql_conformance::vector_promotes_a_scalar_to_a_vector(also checks thevaluefield)promql_lowering::count_over_time_is_count_intenttime_range::q15_count_over_time(full DAG equality)promql_lowering::stddev_and_stdvar_over_timetime_range::q_stddev_over_time,q_stdvar_over_time(full DAG equality)Assertions that cannot fail, deleted (each test stays)
operator_design_examples.rs:!json.contains("KeepPreAsap"/"ScalarBridge"). Neither name exists anywhere in the workspace.frontend-promql/src/error.rs:!msg.contains("PromQL"), right afterassert_eq!(msg, "unsupported feature: subquery").Dead helper deleted:
asap_types::ir::cse::dag_node_countand its 3 tests. It had no non-test caller here, in ASAPQuery or in ASAPCollector. Its one other test use is nowOperatorNode::reachable(..).len(). The CSE cost-model decision doc names that expression instead.Stale ignored tests deleted. I ran each one first and each still fails.
operator_sharing::project_above_and_scan_below_a_summary_are_both_non_asap_nodes: the IR now guarantees what it checks (Flatten ASAP DAG Operators for Both Pre-ASAP and Post-ASAP IR #468). It fails because the plan holds no summary evaluation. Its two helpers went with it.planner_layering_example1::stage2_physical_asap_optimization_every_candidate_compiles_in_the_physical_planner: it cites the merged fix(ir): top-k sketch readouts return selected rows; Example 1 expectations follow planner output #579, and unconfirmed candidates can't compile by design.planner_layering_example4::stage3_plan_selection_b_rebuilding_every_window_costs_mostand…b_prefers_ingestion_time_tumbling_windows: replaced by…b_cost_models_choose_differentlyand…b_pareto_front_over_memory_disk_and_latency. The acceptance doc's rows are removed.Ignore reasons retargeted. Each test was rerun to confirm its current failure.
count_planning::grouped_count_offers_hydra_but_selection_keeps_per_group_stateandexample1::stage1_logical_asap_optimization_q2_offers_hydra: feat(planner): Pass 1 offers HydraCms for grouped approximate counts #605 → Hydra follow-ups: group totals in the executor, Pass 1 HydraCms candidates, Hydra top-k #703.operator_sharing::each_side_of_union_all_holds_a_summary_estimate: Stage 2 materialization: ingestion vs query time per summary #604 → Pass 2 sharing and Stage 1 coverage parity with the retired MajorPass #580. The exact plan is still selected.weighted_topk_binding::planner_weighted_topk_binds_at_either_deployment_phase: fix(ir): top-k sketch readouts return selected rows; Example 1 expectations follow planner output #579 → Stage 1 whole-expression top-k heap keeps outer partition keys indexed against the absorbed aggregate #639. That open issue describes this exact test's whole-expression heap key indexing, and today the CountSketch variant fails with "invalid keyed evaluation shape". The reason also notes that the unconfirmed Count-Min heap has no non-negative weight proof. The other two ignored tests in this file already cite the open Top-k results drop the item columns (port/job); heavy-hitter candidates resolve columns by name #672.Mechanical renames to current terms:
asap_integration_tests::post_asapasap_integration_tests::physical_exportpost_asap_dag(integration tests, frontend-promql test support)physical_asap_dagat_ingestion_post_asap_dagat_ingestion_physical_asap_dagbind_post_asap_before_executionbind_physical_asap_dag_before_executionpost_asap_summary_projection_survives_recoverysummary_projection_survives_recoveryphysical_dag.rscomment "A literal Fallback"Valuessource"CI: the
testjob now runs the DAG viewer's Python tests (python3 -m unittest test_viewer test_serverintools/dag-viewer, withpy-mini-racer==0.6.0) and its Node test (node --test tools/dag-viewer/stages.test.mjs, from feat(dag-viewer): expandable pane chains, query-path highlighting, queries reaching each node #829). The Python tests that plan a query usetarget/debug/stage_pipeline, which the workspace test build has already linked.Not changed:
promql_numeric_regressions.rs(lines ~242–251) does run on today's main. The signedsum_over_timecase offers aCmsWithHeapcandidate, so the loop enters its branch and checks it. It stays.plan-selection/tests/storage_io.rsandphysical_handoff_cost.rsare left out: refactor(plan-selection): delete the unused physical-DAG, storage and handoff cost estimators #833 deletes both files.How
The tests were deleted by name, each with its attributes and doc comment, after a side-by-side read of each pair. The monthly variants reuse the same assertion through two new helpers,
assert_materialized_outputs_cover_their_consumersandpattern_a_windows_and_offsets, so the CI test and the ignored test check the same invariant. The ratchet is aCoverageRatchet { min_asap_candidates, max_planning_errors }per corpus entry.Key code interfaces added / modified
Test-support and one public helper only. There are no planner, wire or CLI changes.
crates/types/src/ir/cse.rscrates/integration-tests/src/lib.rscrates/frontend-promql/tests/observability/metrics_observability.rsUsage. A developer runs the suites the way CI does:
Before this PR
After this PR
Verification
Run locally at this head, rebased on main with #826 and #829. GitHub CI not awaited.
cargo fmt --all -- --check: clean.cargo clippy --workspace --all-targets --all-features --locked -- -D warnings: clean. Two lints that are already on main inplan-selection/src/pass/components.rswere allowed locally; a newer local clippy flags them.cargo check -p asap-planner --tests --locked: ok.cargo test --workspace --locked: 1839 passed, 0 failed, 34 ignored.cargo test -p asap-planner --no-default-features --features promql --locked: 48 passed, 0 failed, 2 ignored.python3 -m unittest test_viewer test_serverran 86 tests, OK.node --test tools/dag-viewer/stages.test.mjs5/5.--ignoredbefore deletion, and each failed. Each retargeted ignore was rerun to read its current failure.Follow-up: #833 also edits the imports of
crates/executor/tests/physical_dag.rsandweighted_topk_binding.rs(asap_executor::dag::→ real paths). Whichever of the two PRs merges second needs a small rebase.