diff --git a/.github/workflows/scripts/warm-upgrade-sim.sh b/.github/workflows/scripts/warm-upgrade-sim.sh index 0ca8af2c7..600838b58 100755 --- a/.github/workflows/scripts/warm-upgrade-sim.sh +++ b/.github/workflows/scripts/warm-upgrade-sim.sh @@ -69,6 +69,95 @@ if ! PATH="$WORKDIR/base-venv/bin:$PATH" \ exit 1 fi +# A warm installation's git pull requests were settled under the PREVIOUS close +# contract, which preferred `closed_at`: a request CLOSED, reopened and later +# MERGED therefore holds the EARLIER close in `closed_on`. The rows below are +# that state, and the assertion after step 2 is that the upgrade recovered the +# reported close from the SOURCE rather than copying the settled one — the +# difference between a 10-hour merge and a 2-hour one, on data no resync has +# touched. #3362 +seed_git_close_time_warm_state() { + echo "=== Seeding the pre-contract git pull-request state ===" + source "$REPO_ROOT/src/ingestion/scripts/lib/ch-exec.sh" + run_ch <<'SQL' +-- TWO generations of the same request, the shape bronze actually holds: the +-- first collection saw it closed, the second saw the merge. The backfill has to +-- answer from the later one, so a lookup that takes whichever row a part merge +-- left behind fails here. +INSERT INTO bronze_github.pull_requests + (unique_key, tenant_id, source_id, state, created_at, closed_at, merged_at, + _airbyte_raw_id, _airbyte_extracted_at, _airbyte_meta, _airbyte_generation_id) +VALUES + ('warm-gh-reopened', 'warm', 'warm', 'closed', + '2026-01-01T00:00:00Z', '2026-01-01T02:00:00Z', '', + 'warm-raw-1', '2026-01-01 03:00:00', '{}', 0), + ('warm-gh-reopened', 'warm', 'warm', 'closed', + '2026-01-01T00:00:00Z', '2026-01-01T02:00:00Z', '2026-01-01T10:00:00Z', + 'warm-raw-2', '2026-01-02 00:00:00', '{}', 0); +-- The pre-#3250 GitLab stream, which only an upgrading installation has, beside +-- the stream that replaced it. Both name the same request and disagree; the +-- current stream has to win by declaration, not by extraction time, so the +-- legacy row here is deliberately the MORE recently extracted of the two. +CREATE TABLE IF NOT EXISTS bronze_gitlab.merge_requests +( + unique_key Nullable(String), tenant_id Nullable(String), source_id Nullable(String), + state Nullable(String), created_at Nullable(String), closed_at Nullable(String), + merged_at Nullable(String), _airbyte_raw_id String, + _airbyte_extracted_at DateTime64(3), _airbyte_meta String, _airbyte_generation_id UInt32 +) +ENGINE = MergeTree ORDER BY _airbyte_raw_id; +INSERT INTO bronze_gitlab.merge_requests + (unique_key, tenant_id, source_id, state, created_at, closed_at, merged_at, + _airbyte_raw_id, _airbyte_extracted_at, _airbyte_meta, _airbyte_generation_id) +VALUES + ('warm-gl-reopened', 'warm', 'warm', 'closed', + '2026-01-04T00:00:00Z', '2026-01-04T01:00:00Z', '', + 'warm-raw-legacy', '2026-01-09 00:00:00', '{}', 0); +INSERT INTO bronze_gitlab.pull_requests + (unique_key, tenant_id, source_id, state, created_at, closed_at, merged_at, + _airbyte_raw_id, _airbyte_extracted_at, _airbyte_meta, _airbyte_generation_id) +VALUES + ('warm-gl-reopened', 'warm', 'warm', 'merged', + '2026-01-04T00:00:00Z', '2026-01-04T01:00:00Z', '2026-01-04T09:00:00Z', + 'warm-raw-new', '2026-01-05 00:00:00', '{}', 0); +INSERT INTO silver.class_git_pull_requests + (tenant_id, source_id, unique_key, pr_id, state, created_on, closed_on, data_source, _version) +VALUES + ('warm', 'warm', 'warm-gh-reopened', 9001, 'MERGED', + '2026-01-01 00:00:00', '2026-01-01 02:00:00', 'insight_github', 1), + ('warm', 'warm', 'warm-gl-reopened', 9003, 'MERGED', + '2026-01-04 00:00:00', '2026-01-04 01:00:00', 'insight_gitlab', 1), + ('warm', 'warm', 'warm-gh-open', 9004, 'OPEN', + '2026-01-06 00:00:00', NULL, 'insight_github', 1), + ('warm', 'warm', 'warm-bb-recovered', 9002, 'MERGED', + '2026-01-05 00:00:00', '2026-01-05 07:00:00', 'insight_bitbucket_cloud', 1); +SQL +} + +assert_git_close_time_recovered() { + echo "=== Asserting the reported close came from the source, not from closed_on ===" + source "$REPO_ROOT/src/ingestion/scripts/lib/ch-exec.sh" + local got + local expected="2026-01-01 10:00:00|2026-01-04 09:00:00|1|1" + # ifNull around each part: concat() propagates a NULL, so an unfilled column + # would otherwise collapse the whole answer to one '\N' and hide WHICH row + # the rollout failed to reach. + got="$(printf "SELECT concat( + ifNull(toString(maxIf(closed_on_reported, unique_key = 'warm-gh-reopened')), 'NULL'), '|', + ifNull(toString(maxIf(closed_on_reported, unique_key = 'warm-gl-reopened')), 'NULL'), '|', + toString(countIf(unique_key = 'warm-bb-recovered' AND closed_on_reported IS NULL)), '|', + toString(countIf(unique_key = 'warm-gh-open' AND closed_on_reported IS NULL))) + FROM silver.class_git_pull_requests + WHERE tenant_id = 'warm'" | _ch_http_query | tr -d '\n')" + if [[ "${got}" != "${expected}" ]]; then + echo "::error title=warm upgrade left a stale close time::Expected '${expected}' — the reopened-then-merged GitHub request reporting its merge from the NEWER bronze generation, the GitLab one reporting the merge the CURRENT stream states rather than the legacy stream's close, and the Bitbucket and still-open rows reporting nothing — but got '${got}'. The rollout must recompute the reported close from bronze under the merged_at-first contract, not copy the settled closed_on — see #3362." + exit 1 + fi + echo "=== Reported close recovered from the source, newest generation and current stream ===" +} + +seed_git_close_time_warm_state + echo "=== Step 2: deploy the working tree onto the warm state ===" dbt_venv_for "$REPO_ROOT/src/ingestion/scripts/bootstrap-db/pins.env" "$WORKDIR/branch-venv" if ! PATH="$WORKDIR/branch-venv/bin:$PATH" \ @@ -77,4 +166,6 @@ if ! PATH="$WORKDIR/branch-venv/bin:$PATH" \ exit 1 fi +assert_git_close_time_recovered + echo "=== Warm upgrade OK: every relation the gold build reads is covered by a migration or heal ===" diff --git a/src/backend/services/analytics/src/domain/metric_definitions/passport.rs b/src/backend/services/analytics/src/domain/metric_definitions/passport.rs index a7d8a3de6..7cc8105b2 100644 --- a/src/backend/services/analytics/src/domain/metric_definitions/passport.rs +++ b/src/backend/services/analytics/src/domain/metric_definitions/passport.rs @@ -19,6 +19,10 @@ const HEADER: &str = "\ Generated from `registry.yaml` by `analytics passports`. Do not edit by hand — regenerate and commit. A drift test (`metric_definitions::passport`) fails when this file and the registry disagree. + +`median(x)` is the textbook median: on an even sample the two middle values are +averaged, so the answer need not be a value any observation took. A percentile +is an order statistic instead, and always answers with one that did. "; /// Render the passport document from the builtin registry. Deterministic: diff --git a/src/backend/services/analytics/src/domain/metric_definitions/passports.md b/src/backend/services/analytics/src/domain/metric_definitions/passports.md index 8cc779266..5d17747ba 100644 --- a/src/backend/services/analytics/src/domain/metric_definitions/passports.md +++ b/src/backend/services/analytics/src/domain/metric_definitions/passports.md @@ -4,6 +4,10 @@ Generated from `registry.yaml` by `analytics passports`. Do not edit by hand — regenerate and commit. A drift test (`metric_definitions::passport`) fails when this file and the registry disagree. +`median(x)` is the textbook median: on an even sample the two middle values are +averaged, so the answer need not be a value any observation took. A percentile +is an order statistic instead, and always answers with one that did. + ## ci.runs — CI runs - Source: ci (ci_metric_observations) @@ -450,7 +454,7 @@ this file and the registry disagree. - Reads: pr_change_size - Formula: median(pr_change_size) - Shape: integer, lower_is_better, unit lines -- Notes: Median diff size of authored pull requests (lines added plus removed). Smaller requests are easier to review. Sources that do not report line counts contribute no values. +- Notes: Median diff size of authored pull requests (lines added plus removed), dated by the day the request was OPENED in UTC and counted whatever state the request reached. Smaller requests are easier to review. A source that never reported line counts contributes no value; a request whose counts were reported as zero — a rename or a mode change — contributes a zero, which is an observed diff of no lines rather than an absence. ## git.pr_commits — Commits per PR @@ -465,16 +469,16 @@ this file and the registry disagree. - Source: git (git_metric_observations) - Reads: pr_cycle_hours - Formula: median(pr_cycle_hours) -- Shape: decimal, lower_is_better, unit h -- Notes: Median hours from opening a pull request to merging it, over requests merged in the period. +- Shape: decimal, neutral, unit h +- Notes: Median hours from opening a pull request to merging it, dated by the merge in UTC, over requests merged in the period. Mostly waiting — for a reviewer, for a build, for someone to press merge — so it describes the path a change travels rather than the person who opened it. A source that does not report a merge time contributes no duration. ## git.pr_cycle_time_p75_h — PR cycle time (p75) - Source: git (git_metric_observations) - Reads: pr_cycle_hours - Formula: p75(pr_cycle_hours) -- Shape: decimal, lower_is_better, unit h -- Notes: 75th percentile of hours from opening a pull request to merging it, over requests merged in the period. +- Shape: decimal, neutral, unit h +- Notes: 75th percentile of hours from opening a pull request to merging it, dated by the merge in UTC, over requests merged in the period. Reads the same waiting as the median and describes the same path, so it too is not a statement about the person who opened the request. ## git.first_review_time_h — Time to first review diff --git a/src/backend/services/analytics/src/domain/metric_definitions/registry.yaml b/src/backend/services/analytics/src/domain/metric_definitions/registry.yaml index ea8cbd468..623f2bdc3 100644 --- a/src/backend/services/analytics/src/domain/metric_definitions/registry.yaml +++ b/src/backend/services/analytics/src/domain/metric_definitions/registry.yaml @@ -1690,7 +1690,7 @@ metrics: - distribution label: PR size description: Typical diff size per pull request - explanation: Median diff size of authored pull requests (lines added plus removed). Smaller requests are easier to review. Sources that do not report line counts contribute no values. + explanation: Median diff size of authored pull requests (lines added plus removed), dated by the day the request was OPENED in UTC and counted whatever state the request reached. Smaller requests are easier to review. A source that never reported line counts contributes no value; a request whose counts were reported as zero — a rename or a mode change — contributes a zero, which is an observed diff of no lines rather than an absence. unit: lines format: integer direction: lower_is_better @@ -1739,10 +1739,10 @@ metrics: label: PR cycle time short_label: PR cycle description: Typical hours from open to merge - explanation: Median hours from opening a pull request to merging it, over requests merged in the period. + explanation: Median hours from opening a pull request to merging it, dated by the merge in UTC, over requests merged in the period. Mostly waiting — for a reviewer, for a build, for someone to press merge — so it describes the path a change travels rather than the person who opened it. A source that does not report a merge time contributes no duration. unit: h format: decimal - direction: lower_is_better + direction: neutral entity_type: person computation: median peer_cohort_key: org_unit @@ -1764,10 +1764,10 @@ metrics: label: PR cycle time (p75) short_label: Cycle p75 description: Slow-end hours from open to merge - explanation: 75th percentile of hours from opening a pull request to merging it, over requests merged in the period. + explanation: 75th percentile of hours from opening a pull request to merging it, dated by the merge in UTC, over requests merged in the period. Reads the same waiting as the median and describes the same path, so it too is not a statement about the person who opened the request. unit: h format: decimal - direction: lower_is_better + direction: neutral entity_type: person computation: !percentile q: 0.75 diff --git a/src/backend/services/analytics/src/domain/metric_results/compiler.rs b/src/backend/services/analytics/src/domain/metric_results/compiler.rs index c3e2b0eae..633ac11c6 100644 --- a/src/backend/services/analytics/src/domain/metric_results/compiler.rs +++ b/src/backend/services/analytics/src/domain/metric_results/compiler.rs @@ -51,6 +51,20 @@ pub(crate) const ACCOUNT_ASSIGNMENT_RELATION: &str = "identity.account_assignmen /// nobody and never falls through to the email map. const EXCLUDED_PERSON_ID: &str = "ffffffff-ffff-ffff-ffff-ffffffffffff"; +/// The median a reader means: the average of the two middle values on an even +/// sample. `quantileExact` takes an index instead, so it answers with the upper +/// middle — a person with two observations is reported at the slower of them, +/// and on a `lower_is_better` metric that always reads unfavourably. +/// `quantileExactInclusive` interpolates to the textbook definition and stays +/// exact, so nothing is approximated by a sketch. A percentile keeps +/// `quantileExact`: unlike the median it has no second definition to match. +/// #3362 +const MEDIAN_AGGREGATE: &str = "quantileExactInclusiveIf"; + +/// `MEDIAN_AGGREGATE` with the `OrNull` combinator, for the arms that must +/// answer an empty window with null rather than the type's default. +const MEDIAN_AGGREGATE_OR_NULL: &str = "quantileExactInclusiveIfOrNull"; + /// Columns a resolved observation subquery re-exposes to the query above it. /// `entity_id` is absent: the subquery replaces it with the canonical person id, /// so every outer clause reads unchanged. @@ -898,7 +912,7 @@ fn grouped_value_expr(def: &MetricDefinition) -> String { ) } ComputationSpec::Median { .. } => { - "quantileExactIf(0.5)(value, value IS NOT NULL)".to_owned() + format!("{MEDIAN_AGGREGATE}(0.5)(value, value IS NOT NULL)") } ComputationSpec::Percentile { q, .. } => { format!("quantileExactIf({q})(value, value IS NOT NULL)") @@ -945,7 +959,7 @@ fn grouped_value_expr_within( } ComputationSpec::Median { .. } => { let window = window_term(window, params); - format!("quantileExactIf(0.5)(value, value IS NOT NULL{window})") + format!("{MEDIAN_AGGREGATE}(0.5)(value, value IS NOT NULL{window})") } ComputationSpec::Percentile { q, .. } => { let window = window_term(window, params); @@ -1349,13 +1363,17 @@ fn push_peer_stat_selects(selects: &mut String, item_index: usize) { let aliases = peer_aliases(item_index); let observed = format!("peer.{value} IS NOT NULL"); let pool = format!("uniqExactIf(peer.entity_id, {observed})"); - let quantiles = format!("quantilesExactIf(0.25, 0.5, 0.75)(peer.{value}, {observed})"); + // p25 and p75 are order statistics and keep `quantilesExact`; the peer + // median answers the same question as the period one and so reads the same + // textbook definition, or a cohort of an even size disagrees with itself. + let quantiles = format!("quantilesExactIf(0.25, 0.75)(peer.{value}, {observed})"); + let median_expr = format!("{MEDIAN_AGGREGATE}(0.5)(peer.{value}, {observed})"); let _ = write!( selects, ", if({pool} >= {min_peer_n}, toNullable({quantiles}[1]), NULL) AS {p25}, - if({pool} >= {min_peer_n}, toNullable({quantiles}[2]), NULL) AS {median}, - if({pool} >= {min_peer_n}, toNullable({quantiles}[3]), NULL) AS {p75}, + if({pool} >= {min_peer_n}, toNullable({median_expr}), NULL) AS {median}, + if({pool} >= {min_peer_n}, toNullable({quantiles}[2]), NULL) AS {p75}, if({pool} >= {min_peer_n}, minIfOrNull(peer.{value}, {observed}), NULL) AS {min}, if({pool} >= {min_peer_n}, maxIfOrNull(peer.{value}, {observed}), NULL) AS {max}, toUInt64({pool}) AS {n}", @@ -1449,7 +1467,7 @@ fn item_value_expr( params.push(value.measure_key.clone()); let window = window_term(window, params); format!( - "quantileExactIfOrNull(0.5)(value, source_key = ? AND measure_key = ? AND value IS NOT NULL{window})" + "{MEDIAN_AGGREGATE_OR_NULL}(0.5)(value, source_key = ? AND measure_key = ? AND value IS NOT NULL{window})" ) } ComputationSpec::Percentile { value, q } => { @@ -3321,14 +3339,22 @@ mod tests { ))); assert!(query.sql.contains(&format!("AS m{item}_target"))); } - // Quartiles come from one `quantilesExactIf` per item (single sort), - // not three separate `quantileExactIf` calls. + // The two order statistics come from one `quantilesExactIf` per item + // (single sort), not two separate `quantileExactIf` calls, and the peer + // median reads the same textbook aggregate as every other median view. for item in 0..2 { assert!(query.sql.contains(&format!( - "quantilesExactIf(0.25, 0.5, 0.75)(peer.m{item}, peer.m{item} IS NOT NULL)" + "quantilesExactIf(0.25, 0.75)(peer.m{item}, peer.m{item} IS NOT NULL)" + ))); + assert!(query.sql.contains(&format!( + "quantileExactInclusiveIf(0.5)(peer.m{item}, peer.m{item} IS NOT NULL)" ))); } assert!(!query.sql.contains("quantileExactIf(0.25)")); + assert!( + !query.sql.contains("quantilesExactIf(0.25, 0.5, 0.75)"), + "the peer median must not come back as the middle order statistic" + ); // The cohort relation is canonical-grained (one row per person and // cohort_key, contested membership already dropped), so the pool reads // it straight. A collapse here would repair a broken input silently. @@ -3419,9 +3445,10 @@ mod tests { ] { assert!( query.sql.contains( - "quantileExactIfOrNull(0.5)(value, source_key = ? AND measure_key = ?" + "quantileExactInclusiveIfOrNull(0.5)(value, source_key = ? AND measure_key = ?" ), - "median must batch as an OrNull quantile column" + "median must batch as an OrNull quantile column, interpolating both \ + middle values on an even sample" ); assert_eq!(query.sql.matches('?').count(), query.params.len()); } @@ -3439,13 +3466,13 @@ mod tests { ); assert!( ts.sql - .contains("quantileExactIf(0.5)(value, value IS NOT NULL)") + .contains("quantileExactInclusiveIf(0.5)(value, value IS NOT NULL)") ); assert!(ts.sql.contains("GROUP BY GROUPING SETS")); let bd = compile_breakdown_query(&median_metric(), &request(), &["source".to_owned()], &[]); assert!( bd.sql - .contains("quantileExactIf(0.5)(value, value IS NOT NULL)") + .contains("quantileExactInclusiveIf(0.5)(value, value IS NOT NULL)") ); } diff --git a/src/ingestion/connectors/git/bitbucket-cloud/dbt/bitbucket_cloud__pull_requests.sql b/src/ingestion/connectors/git/bitbucket-cloud/dbt/bitbucket_cloud__pull_requests.sql index 78af36700..1b55fe916 100644 --- a/src/ingestion/connectors/git/bitbucket-cloud/dbt/bitbucket_cloud__pull_requests.sql +++ b/src/ingestion/connectors/git/bitbucket-cloud/dbt/bitbucket_cloud__pull_requests.sql @@ -10,21 +10,61 @@ -- Bitbucket carries no diff totals on the pull request itself, so the per-file -- diffstat rows are the only source of line counts and are summed here. -WITH diff_stats AS ( +-- +-- INVARIANT: the row key is the file path, so a ReplacingMergeTree replaces a +-- file's row and never removes one — a file a rebase dropped out of the diff +-- keeps its row for ever. The parent's update stamp is the only thing saying +-- which rows belong to the diff the request has NOW, so the newest stamp's rows +-- are taken WHOLE. Resolving each file to its own newest row keeps the dropped +-- file instead, because nothing newer exists to displace it. #3362 +-- +-- A row with no usable stamp sorts to the epoch, so a request whose rows all +-- predate the stamp is summed entire, as it was before this rule. #3362 +WITH diffstat_rows AS ( SELECT tenant_id, source_id, repo_full_name, pr_id, - count() AS files_changed, - sum(lines_added) AS lines_added, - sum(lines_removed) AS lines_removed, - max(_airbyte_extracted_at) AS _airbyte_extracted_at, - 1 AS matched + COALESCE(parseDateTimeBestEffortOrNull(pr_updated_on), toDateTime(0)) AS generation, + lines_added, + lines_removed, + _airbyte_extracted_at FROM {{ source('bronze_bitbucket_cloud', 'pull_request_diffstat') }} FINAL +), + +diffstat_newest_generation AS ( + SELECT + tenant_id, + source_id, + repo_full_name, + pr_id, + max(generation) AS generation + FROM diffstat_rows GROUP BY tenant_id, source_id, repo_full_name, pr_id ), +diff_stats AS ( + SELECT + stat.tenant_id AS tenant_id, + stat.source_id AS source_id, + stat.repo_full_name AS repo_full_name, + stat.pr_id AS pr_id, + count() AS files_changed, + sum(stat.lines_added) AS lines_added, + sum(stat.lines_removed) AS lines_removed, + max(stat._airbyte_extracted_at) AS _airbyte_extracted_at, + 1 AS matched + FROM diffstat_rows AS stat + INNER JOIN diffstat_newest_generation AS newest + ON newest.tenant_id = stat.tenant_id + AND newest.source_id = stat.source_id + AND newest.repo_full_name = stat.repo_full_name + AND newest.pr_id = stat.pr_id + WHERE stat.generation = newest.generation + GROUP BY stat.tenant_id, stat.source_id, stat.repo_full_name, stat.pr_id +), + -- A pull request records no close time of its own; the terminal update event -- in its activity does. terminal_activity AS ( @@ -177,6 +217,17 @@ SELECT AND COALESCE(updated_on >= created_on, 1), updated_on, CAST(NULL AS Nullable(DateTime)) ) AS closed_on, + -- The close time as the SOURCE stated it: the terminal entry in the + -- request's activity, never a recovered candidate. The recovery above + -- corroborates WHICH DAY a merge landed on, which is what a count needs; a + -- duration measured to it would report an interval nobody observed, so the + -- duration measures read this column and drop the request when it is null. + -- #3362 + if( + COALESCE(pr.state, '') IN ('MERGED', 'DECLINED', 'SUPERSEDED'), + parseDateTimeBestEffortOrNull(activity.closed_on), + CAST(NULL AS Nullable(DateTime)) + ) AS closed_on_reported, COALESCE(pr.merge_commit_sha, '') AS merge_commit_hash, -- An unmatched join partner and a genuinely empty pull request both read -- as 0 through COALESCE; only the marker separates "not collected yet" diff --git a/src/ingestion/connectors/git/github/dbt/github__pull_requests.sql b/src/ingestion/connectors/git/github/dbt/github__pull_requests.sql index 0956bcb5b..f1431541f 100644 --- a/src/ingestion/connectors/git/github/dbt/github__pull_requests.sql +++ b/src/ingestion/connectors/git/github/dbt/github__pull_requests.sql @@ -55,7 +55,17 @@ SELECT COALESCE(pr.base_ref, '') AS destination_branch, parseDateTimeBestEffortOrNull(pr.created_at) AS created_on, parseDateTimeBestEffortOrNull(pr.updated_at) AS updated_on, - parseDateTimeBestEffortOrNull(COALESCE(pr.closed_at, pr.merged_at)) AS closed_on, + -- A request that merged closed when it merged. `closed_at` describes one + -- closed WITHOUT merging, and a request closed, reopened and then merged + -- keeps that earlier close — taking it would end the interval before the + -- merge and date the merge to the wrong day. + parseDateTimeBestEffortOrNull( + COALESCE(nullIf(pr.merged_at, ''), nullIf(pr.closed_at, '')) + ) AS closed_on, + -- GitHub reports the close time itself, so there is nothing to derive. + parseDateTimeBestEffortOrNull( + COALESCE(nullIf(pr.merged_at, ''), nullIf(pr.closed_at, '')) + ) AS closed_on_reported, COALESCE(pr.merge_commit_sha, '') AS merge_commit_hash, -- An unmatched join partner and a genuinely empty pull request both read -- as 0 through COALESCE; only the marker separates "not collected yet" diff --git a/src/ingestion/connectors/git/gitlab/dbt/gitlab__pull_requests.sql b/src/ingestion/connectors/git/gitlab/dbt/gitlab__pull_requests.sql index b65ea8d95..070054359 100644 --- a/src/ingestion/connectors/git/gitlab/dbt/gitlab__pull_requests.sql +++ b/src/ingestion/connectors/git/gitlab/dbt/gitlab__pull_requests.sql @@ -84,6 +84,9 @@ SELECT parseDateTimeBestEffortOrNull(mr.created_at) AS created_on, parseDateTimeBestEffortOrNull(mr.updated_at) AS updated_on, parseDateTimeBestEffortOrNull(COALESCE(NULLIF(mr.merged_at, ''), mr.closed_at)) AS closed_on, + -- GitLab states the close time itself, so the reported column carries the + -- same value: there is nothing derived here for a duration to read past. + parseDateTimeBestEffortOrNull(COALESCE(NULLIF(mr.merged_at, ''), mr.closed_at)) AS closed_on_reported, -- A squash merge lands as squash_commit_sha and leaves merge_commit_sha -- empty; either is the commit the target branch received. COALESCE(NULLIF(mr.merge_commit_sha, ''), mr.squash_commit_sha, '') AS merge_commit_hash, diff --git a/src/ingestion/gold/git_metric_evidence.sql b/src/ingestion/gold/git_metric_evidence.sql index 8c34dd2a4..35711ccdd 100644 --- a/src/ingestion/gold/git_metric_evidence.sql +++ b/src/ingestion/gold/git_metric_evidence.sql @@ -328,6 +328,12 @@ pull_requests_source AS ( prs.state AS state, prs.created_on AS created_on, prs.closed_on AS closed_on, + -- INVARIANT: the close time the SOURCE stated, which is not always the + -- one the class row settles on — Bitbucket recovers a merge the vendor + -- recorded without an activity entry. A count wants the settled day; a + -- duration wants an instant somebody observed, so every interval below + -- reads this column and drops the request where it is null. #3362 + prs.closed_on_reported AS closed_on_reported, coalesce(pr_commit_counts.linked_commit_count, 0) AS linked_commit_count, coalesce(review_summary.reviewer_count, 0) AS reviewer_count, coalesce(review_summary.has_approval, 0) AS has_approval, @@ -336,10 +342,10 @@ pull_requests_source AS ( prs.lines_added + prs.lines_removed AS change_size, if( prs.state = 'MERGED' - AND prs.closed_on IS NOT NULL + AND prs.closed_on_reported IS NOT NULL AND prs.created_on IS NOT NULL - AND prs.closed_on >= prs.created_on, - dateDiff('second', prs.created_on, prs.closed_on) / 3600.0, + AND prs.closed_on_reported >= prs.created_on, + dateDiff('second', prs.created_on, prs.closed_on_reported) / 3600.0, CAST(NULL AS Nullable(Float64)) ) AS cycle_hours, if( @@ -351,18 +357,18 @@ pull_requests_source AS ( ) AS first_review_hours, if( prs.state = 'MERGED' - AND prs.closed_on IS NOT NULL + AND prs.closed_on_reported IS NOT NULL AND review_summary.first_reviewed_at IS NOT NULL - AND prs.closed_on >= review_summary.first_reviewed_at, - dateDiff('second', review_summary.first_reviewed_at, prs.closed_on) / 3600.0, + AND prs.closed_on_reported >= review_summary.first_reviewed_at, + dateDiff('second', review_summary.first_reviewed_at, prs.closed_on_reported) / 3600.0, CAST(NULL AS Nullable(Float64)) ) AS review_to_merge_hours, if( prs.state = 'MERGED' - AND prs.closed_on IS NOT NULL + AND prs.closed_on_reported IS NOT NULL AND review_summary.last_approved_at IS NOT NULL - AND prs.closed_on >= review_summary.last_approved_at, - dateDiff('second', review_summary.last_approved_at, prs.closed_on) / 3600.0, + AND prs.closed_on_reported >= review_summary.last_approved_at, + dateDiff('second', review_summary.last_approved_at, prs.closed_on_reported) / 3600.0, CAST(NULL AS Nullable(Float64)) ) AS approval_to_merge_hours, if(coalesce(prs.project_key, '') = '', '__unknown__', concat(coalesce(toString(prs.source_id), ''), ':', prs.project_key)) AS project_value, @@ -490,9 +496,14 @@ pull_request_measures AS ( [tuple('pr_multi_reviewed', toFloat64(reviewer_count > 1), toDateTime64(assumeNotNull(created_on), 3))], [] ), - if( - created_on IS NOT NULL AND ifNull(change_size, 0) > 0, - [tuple('pr_change_size', toFloat64(ifNull(change_size, 0)), toDateTime64(assumeNotNull(created_on), 3))], + -- A source that never reported line counts leaves them null and + -- contributes nothing; a source that reported zero of each observed a + -- diff of no lines — a rename or a mode change — and that zero is a + -- value. Only the null separates the two, which is why the class + -- columns are nullable. #3362 + if( + created_on IS NOT NULL AND change_size IS NOT NULL, + [tuple('pr_change_size', toFloat64(assumeNotNull(change_size)), toDateTime64(assumeNotNull(created_on), 3))], [] ), if( @@ -523,8 +534,8 @@ pull_request_measures AS ( [] ), if( - cycle_hours IS NOT NULL AND closed_on IS NOT NULL, - [tuple('pr_cycle_hours', toFloat64(assumeNotNull(cycle_hours)), toDateTime64(assumeNotNull(closed_on), 3))], + cycle_hours IS NOT NULL AND closed_on_reported IS NOT NULL, + [tuple('pr_cycle_hours', toFloat64(assumeNotNull(cycle_hours)), toDateTime64(assumeNotNull(closed_on_reported), 3))], [] ), if( @@ -533,13 +544,13 @@ pull_request_measures AS ( [] ), if( - review_to_merge_hours IS NOT NULL AND closed_on IS NOT NULL, - [tuple('pr_review_to_merge_hours', toFloat64(assumeNotNull(review_to_merge_hours)), toDateTime64(assumeNotNull(closed_on), 3))], + review_to_merge_hours IS NOT NULL AND closed_on_reported IS NOT NULL, + [tuple('pr_review_to_merge_hours', toFloat64(assumeNotNull(review_to_merge_hours)), toDateTime64(assumeNotNull(closed_on_reported), 3))], [] ), if( - approval_to_merge_hours IS NOT NULL AND closed_on IS NOT NULL, - [tuple('pr_approval_to_merge_hours', toFloat64(assumeNotNull(approval_to_merge_hours)), toDateTime64(assumeNotNull(closed_on), 3))], + approval_to_merge_hours IS NOT NULL AND closed_on_reported IS NOT NULL, + [tuple('pr_approval_to_merge_hours', toFloat64(assumeNotNull(approval_to_merge_hours)), toDateTime64(assumeNotNull(closed_on_reported), 3))], [] ), if( @@ -547,8 +558,8 @@ pull_request_measures AS ( AND review_to_merge_hours IS NOT NULL AND cycle_hours IS NOT NULL AND cycle_hours > 0 - AND closed_on IS NOT NULL, - [tuple('pr_review_wait_share', 100.0 * toFloat64(assumeNotNull(first_review_hours)) / toFloat64(assumeNotNull(cycle_hours)), toDateTime64(assumeNotNull(closed_on), 3))], + AND closed_on_reported IS NOT NULL, + [tuple('pr_review_wait_share', 100.0 * toFloat64(assumeNotNull(first_review_hours)) / toFloat64(assumeNotNull(cycle_hours)), toDateTime64(assumeNotNull(closed_on_reported), 3))], [] ) ) AS Array(Tuple(measure_key String, contribution Float64, observed_at DateTime64(3)))) AS pr_measure diff --git a/src/ingestion/scripts/apply-ch-migrations.sh b/src/ingestion/scripts/apply-ch-migrations.sh index c5dcaec3f..644957dcb 100755 --- a/src/ingestion/scripts/apply-ch-migrations.sh +++ b/src/ingestion/scripts/apply-ch-migrations.sh @@ -556,6 +556,177 @@ for _git_source in github gitlab bitbucket_cloud; do heal_git_pr_author_account "${_git_source}__pull_requests" done +echo "=== Healing git pull-request reported close-time column ===" +# Same positional invariant: every projection feeding class_git_pull_requests +# gained closed_on_reported after closed_on — the close time as the source +# stated it, which the duration measures read so a recovered one cannot pose as +# a measurement (#3362). Staging heals here because these tables exist only +# after a connector has run; the silver column is added in migrations/*.sql. +# Idempotent. +heal_git_pr_close_time_reported() { + local table="$1" + ch_table_is_real staging "${table}" || return 0 + echo " staging.${table}" + run_ch < ". The +# class holds all three connectors in one table, so it alone needs the filter +# that keeps Bitbucket out; the staging projections are per connector already. +_GIT_PR_REPORTED_CLOSE_TARGETS=( + "silver|class_git_pull_requests| AND data_source IN ('insight_github', 'insight_gitlab')" + "staging|github__pull_requests|" + "staging|gitlab__pull_requests|" +) + +# Rows this backfill would actually CHANGE. `fillable` is the same expression +# the UPDATE below uses, so a converged warehouse counts zero and issues no +# mutation at all — the rows that stay NULL for ever (a merge bronze cannot +# answer for, a request still open) must not re-trigger it on every deploy. +_git_pr_rows_needing_reported_close() { + local db="$1" table="$2" predicate="$3" fillable="$4" + printf "SELECT count() FROM %s.%s WHERE closed_on_reported IS NULL AND unique_key IS NOT NULL AND tenant_id IS NOT NULL AND source_id IS NOT NULL AND (%s)%s" \ + "${db}" "${table}" "${fillable}" "${predicate}" | + _ch_http_query | tr -d '[:space:]' +} + +# One branch of the lookup's UNION: the reported close as the source states it. +# +# `stream_rank` settles which relation wins when a GitLab installation holds +# both the pre-#3250 stream and its replacement. The current stream ranks above +# the legacy one by DECLARATION, not by whichever happened to be extracted +# later — a clock is not a contract. +_git_pr_reported_close_branch() { + local relation="$1" stream_rank="$2" + cat < ": the current streams outrank the legacy one. + for source in "bronze_github.pull_requests 1" "bronze_gitlab.pull_requests 1" \ + "bronze_gitlab.merge_requests 0"; do + relation="${source%% *}" + ch_table_is_real "${relation%%.*}" "${relation##*.}" && sources+=("${source}") + done + [[ "${#sources[@]}" -gt 0 ]] || { echo " no git pull-request bronze to read — skipping"; return 0; } + + branches="" + for source in "${sources[@]}"; do + relation="${source%% *}"; rank="${source##* }" + [[ -n "${branches}" ]] && branches+=" UNION ALL"$'\n' + branches+="$(_git_pr_reported_close_branch "${relation}" "${rank}")"$'\n' + done + + run_ch < + The close time as the source stated it, and nothing derived. NULL when + the source stated none: Bitbucket recovers such a merge into + `closed_on` so a count still keeps it, and every duration measure + reads this column instead, so a recovered time cannot pose as a + measured interval. + - name: files_changed + description: > + NULL when the diff summary has not been collected or the source has + not computed it yet, 0 when it was collected and touched no file. + - name: lines_added + description: > + NULL when the diff summary has not been collected or the source has + not computed it yet, 0 when it was collected and added no line. + - name: lines_removed + description: > + NULL when the diff summary has not been collected or the source has + not computed it yet, 0 when it was collected and removed no line. - name: class_git_pull_requests_reviewers description: "Unified git pull request reviewers from all sources (GitHub, Bitbucket, GitLab)" diff --git a/tests/datapath/meta/test_bronze_schemas_match_the_ddl_snapshot.py b/tests/datapath/meta/test_bronze_schemas_match_the_ddl_snapshot.py new file mode 100644 index 000000000..57306877a --- /dev/null +++ b/tests/datapath/meta/test_bronze_schemas_match_the_ddl_snapshot.py @@ -0,0 +1,104 @@ +"""The bronze test schemas against the DDL snapshot they are declared to come from. + +A spec can seed only what its table's schema declares, so a schema that has drifted +from the snapshot silently makes a real source state unseedable — a pull request with +no merge time, or a diffstat row's collection stamp. Two such gaps had to be closed +before the pull-request metrics could be tested at all (#3362), and neither showed up +as a failure: every existing spec happened to set the columns involved. + +Two rules, and deliberately not a third: + +* a column no snapshot table has is always a mistake, so that is checked everywhere; +* exact column parity is a RATCHET over `PARITY_TABLES` — those hold it today and + must keep it. Most schemas are partial by choice and bringing them in is separate + work, so they are not listed. + +Nullability is left alone on purpose. Declaring an identity column non-null is what +forces a spec to state it, and the columns widened in #3362 were widened because a +real source state needed them, which no rule derived from the snapshot can tell. +""" + +from __future__ import annotations + +import re +from pathlib import Path + +import pytest +import yaml + +REPO_ROOT = Path(__file__).resolve().parents[3] +SCHEMA_DIR = REPO_ROOT / "tests/datapath/metrics/schemas" +DDL_DIR = REPO_ROOT / "src/ingestion/scripts/connectors-ddl" + +#: Tables whose test schema mirrors the snapshot column for column. A table joins +#: this list once its schema is complete; it never leaves. +PARITY_TABLES = ( + "bronze_bitbucket_cloud.pull_requests", + "bronze_bitbucket_cloud.pull_request_diffstat", + "bronze_gitlab.pull_requests", +) + +_CREATE = re.compile( + r"CREATE TABLE IF NOT EXISTS\s+(?P[\w.]+)\s*\((?P.*?)\)\s*ENGINE", + re.DOTALL, +) +_COLUMN = re.compile(r"^\s*`(?P[^`]+)`\s+(?P.+?),?\s*$") + + +def _snapshot_columns() -> dict[str, set[str]]: + """Every `bronze_*` table the generated snapshot declares, and its columns.""" + tables: dict[str, set[str]] = {} + for sql in sorted(DDL_DIR.glob("*.sql")): + for create in _CREATE.finditer(sql.read_text(encoding="utf-8")): + columns = { + match.group("col") + for match in (_COLUMN.match(line) for line in create.group("body").splitlines()) + if match + } + tables[create.group("name")] = columns + return tables + + +def _declared_columns() -> dict[str, set[str]]: + """Every table a data-path schema file declares, and the columns it allows.""" + declared: dict[str, set[str]] = {} + for path in sorted(SCHEMA_DIR.glob("*.yaml")): + document = yaml.safe_load(path.read_text(encoding="utf-8")) or {} + for table, spec in (document.get("schemas") or {}).items(): + declared[table] = set((spec.get("properties") or {}).keys()) + return declared + + +SNAPSHOT = _snapshot_columns() +DECLARED = _declared_columns() + + +def test_the_snapshot_parses() -> None: + """A snapshot this file cannot read would make every rule below vacuous.""" + assert len(SNAPSHOT) > 100, f"only {len(SNAPSHOT)} tables parsed out of the snapshot" + assert DECLARED, "no data-path schema files were found" + + +@pytest.mark.parametrize("table", sorted(DECLARED)) +def test_a_schema_declares_no_column_the_snapshot_lacks(table: str) -> None: + """A column outside the real table cannot be seeded and is a typo or a rename.""" + if table not in SNAPSHOT: + pytest.skip(f"{table} is not in the connectors DDL snapshot") + unknown = sorted(DECLARED[table] - SNAPSHOT[table]) + assert not unknown, f"{table}: declared but absent from the snapshot: {unknown}" + + +@pytest.mark.parametrize("table", PARITY_TABLES) +def test_a_parity_table_declares_every_column_the_snapshot_has(table: str) -> None: + """The ratchet: a column added to one of these tables must reach its schema. + + Without it a spec cannot seed the new column, and nothing fails — the gap only + surfaces when somebody tries to write the test that needs it. + """ + assert table in SNAPSHOT, f"{table} is not in the connectors DDL snapshot" + assert table in DECLARED, f"{table} has no data-path schema file" + missing = sorted(SNAPSHOT[table] - DECLARED[table]) + assert not missing, ( + f"{table}: in the snapshot but not declared: {missing}. Add them to " + f"tests/datapath/metrics/schemas/{table}.yaml" + ) diff --git a/tests/datapath/metrics/git/git_bitbucket_merge_time_recovery.test.yaml b/tests/datapath/metrics/git/git_bitbucket_merge_time_recovery.test.yaml index fb6e93609..256702d2a 100644 --- a/tests/datapath/metrics/git/git_bitbucket_merge_time_recovery.test.yaml +++ b/tests/datapath/metrics/git/git_bitbucket_merge_time_recovery.test.yaml @@ -18,8 +18,9 @@ description: > account for it, and nothing when neither is at or after the creation • gold: a request in the MERGED state with a known close time contributes exactly 1 to git.prs_merged, dated by the CLOSE — so a merge with no - close time reaches no period at all. The same close time feeds the - duration measures, which discard an interval that does not run forwards + close time reaches no period at all. A RECOVERED close dates that count + and nothing more: the duration measures read the close the source itself + stated, so a request the recovery dated contributes no interval (#3362) Cases, one request each: the terminal entry wins even when the merge commit sits on another day; a silent close with no other activity is dated by the @@ -37,8 +38,8 @@ description: > gain no close time, or it would count as an abandonment that never happened. Beyond git.prs_merged the module reads git.prs_created as a vacuity guard, - git.pr_cycle_time_h to prove a recovered close is usable and not merely - countable, and git.pr_abandonment_rate for the DECLINED guard. + git.pr_cycle_time_h to prove a recovered close is countable and NOT usable as + a duration, and git.pr_abandonment_rate for the DECLINED guard. The fixture's commits carry no author address on purpose: they exist to carry a date, and an addressless commit reaches no commit or line metric, so diff --git a/tests/datapath/metrics/git/git_commit_size.test.yaml b/tests/datapath/metrics/git/git_commit_size.test.yaml index 4c6edaa69..a698a35cf 100644 --- a/tests/datapath/metrics/git/git_commit_size.test.yaml +++ b/tests/datapath/metrics/git/git_commit_size.test.yaml @@ -11,8 +11,8 @@ description: > Fixture: * erin, 2026-10-01, acme/api: sizes 10, 30, 204 — an odd pool whose median is 30, plus a merge commit (1000) that must contribute nothing. - * erin, 2026-10-02, acme/web: sizes 4, 6 — an even pool; ClickHouse - quantileExact(0.5) takes the UPPER middle element, so the day reads 6. + * erin, 2026-10-02, acme/web: sizes 4, 6 — an even pool, so the day reads + their average, 5, which is a size no commit had. * dave, 2026-10-01, acme/api: one commit whose bronze row carries no stats at all. Staging projects absent stats to zero, so the commit enters the median as 0 rather than abstaining. diff --git a/tests/datapath/metrics/git/git_commit_work_identity.test.yaml b/tests/datapath/metrics/git/git_commit_work_identity.test.yaml index 2ad697d9f..627a718e6 100644 --- a/tests/datapath/metrics/git/git_commit_work_identity.test.yaml +++ b/tests/datapath/metrics/git/git_commit_work_identity.test.yaml @@ -38,7 +38,8 @@ description: > Commit size reads the same commit set: the squash pool is {10, 10, 6} (median 10), the surviving patch copy is 7, the partial-branch pair is - {3, 2} (median 3), and the cross-repository survivor keeps its 4. + {3, 2} (median 2.5, both middles averaged), and the cross-repository + survivor keeps its 4. bronze: bronze_bamboohr.employees: diff --git a/tests/datapath/metrics/git/git_commits_bitbucket.test.yaml b/tests/datapath/metrics/git/git_commits_bitbucket.test.yaml index 3666b2ff9..49ebdb25a 100644 --- a/tests/datapath/metrics/git/git_commits_bitbucket.test.yaml +++ b/tests/datapath/metrics/git/git_commits_bitbucket.test.yaml @@ -12,7 +12,7 @@ description: > * pull-request line totals are summed from the per-file diffstat rows, and closed_on comes from the terminal MERGED activity event; * commit size reads the commits' own stats — {12, 0} with the merge commit - contributing nothing — and the even-count median takes the upper middle. + contributing nothing — and the even-count median is their average, 6. bronze: bronze_bamboohr.employees: diff --git a/tests/datapath/metrics/git/git_pr_commits.test.yaml b/tests/datapath/metrics/git/git_pr_commits.test.yaml index fe09d3304..dfec7ea8d 100644 --- a/tests/datapath/metrics/git/git_pr_commits.test.yaml +++ b/tests/datapath/metrics/git/git_pr_commits.test.yaml @@ -12,8 +12,9 @@ description: > alice merges three requests with 1, 2, and 4 linked commits (one link row seeded twice as a re-sync duplicate) → median 2, mean 2.33 — a wrong - aggregation fails the equal. bob merges one request with no link rows → null, - and stays out of the peer pool. + aggregation fails the equal. Two of them merge on 2026-10-01, so that day's + even pool {1, 2} reads 1.5, a count no request had. bob merges one request + with no link rows → null, and stays out of the peer pool. Team (median/range = the person's department): per-person values alice 2, carol 3, dave 5, erin 1, heidi 6 → median 3, range [1, 6], n 5. Cases: alice median across views, merge-day dating, bob honest absence, diff --git a/tests/datapath/metrics/git/git_pr_cycle_time.test.yaml b/tests/datapath/metrics/git/git_pr_cycle_time.test.yaml new file mode 100644 index 000000000..2874f7dd1 --- /dev/null +++ b/tests/datapath/metrics/git/git_pr_cycle_time.test.yaml @@ -0,0 +1,148 @@ +spec_version: 1 +description: > + Metric: git.pr_cycle_time_h — typical hours from opening a pull request to + merging it, #3065. + How it's computed (bronze → silver → gold): + • bronze: pull requests carrying the times the source reports — an opening + time, a merge time, and for some sources a separate closing time; on + Bitbucket the merge time is not on the request at all + • silver: one class row per request, carrying an opening time and ONE close + time. For a request that merged, the close time is the moment of the + MERGE; a closing time the source reports separately belongs to a request + that was closed without merging and must not stand in for a merge + • gold: hours from opening to merge, one value per MERGED request, dated + by the merge. A request that never merged contributes nothing, and so + does one whose interval does not run forwards + • serve: quantileExactInclusive(0.5) over the period's values + + Two source shapes are pinned separately. A source reporting BOTH a closing + time and a merge time on a merged request is measured to the merge time: a + request closed, reopened and then merged reports the earlier close, and taking + it would report a cycle that ended before the merge happened. #3065 + + Bitbucket reports no merge time on a request. The terminal entry in its + activity states one where there is one; where there is none the close time is + RECOVERED from the request's own last update, corroborated by a collected + merge commit (#3051, #3328). That recovery answers which DAY a merge landed + on, which is what a count needs — a duration measured to it would report an + interval nobody observed. So the class contract keeps the stated time apart in + its own column and this metric reads only that: a silently merged request + counts in git.prs_merged and contributes NO cycle. #3362 + + The median is the textbook one — on an even sample both middle values are + averaged, so the answer need not be a duration any request took. #3362 + + alice merges five requests with cycles [2, 6, 10, 30, 100] h, so her period + median is 10 and a mean (29.6) fails it. The 2026-11-01 bucket holds + [2, 6, 10] and serves 6. + dave's single request was closed 2 h after opening and merged 8 h after that, + so its cycle is 10 h, not 2. + bob's GitLab requests run 8 h and 9 h, the second reporting a close 1 h in + that must not be taken for the merge; his period medians the pair to 8.5. + heidi's request with a terminal activity entry runs 12 h; her silently merged + one has no stated close time and so no cycle, though it still counts merged. + carol's requests never merged and erin's merge is recorded before her opening, + so neither contributes — which their created counts prove is not absence of + the request. + + heidi is reached the way a real installation reaches her, not by a seeded + binding: her requests name her by account id alone, the connector states that + account as a workspace participant, and the seed attaches it to the person her + collected commit address names (#3423). A Bitbucket assertion here therefore + fails if that path breaks, instead of passing over it. + + Cases: the median across views and the merge-day bucketing; a merged request + is measured to its merge and not to an earlier close, on GitHub and on GitLab; + a Bitbucket request whose close time the source stated against one whose close + time was recovered; an unmerged request contributes nothing; an interval that + runs backwards is dropped rather than served negative, zero, or dated at the + epoch; an empty window. + +identity_accounts: + # GitLab exposes a user's address only to an admin-scoped token, so its + # request reaches a person through the account alone. Bitbucket is NOT bound + # here: it walks the real path — the connector states the account as a + # workspace participant and the seed attaches it to the person its commit + # address names (#3423) — so this spec exercises that path instead of + # standing in for it. + - {source_type: gitlab, source_id: gitlab-test, account_id: "731", person: bob@example.com} + +bronze: + bronze_bamboohr.employees: + - $ref: ../templates/people.yaml#/templates/alice + - $ref: ../templates/people.yaml#/templates/bob + - $ref: ../templates/people.yaml#/templates/carol + - $ref: ../templates/people.yaml#/templates/dave + - $ref: ../templates/people.yaml#/templates/erin + - $ref: ../templates/people.yaml#/templates/heidi + + bronze_github.pull_requests: + # alice: all five opened at 2026-11-01T00:00, merging 2, 6, 10, 30 and 100 + # hours later. Closing time equals the merge, the ordinary GitHub shape. + - {$ref: ../templates/git_activity.yaml#/templates/pull_request, unique_key: ct-a1, id: 701, number: 701, author_login: alice, created_at: "2026-11-01T00:00:00Z", closed_at: "2026-11-01T02:00:00Z", merged_at: "2026-11-01T02:00:00Z", merge_commit_sha: ct-merge-a1} + - {$ref: ../templates/git_activity.yaml#/templates/pull_request, unique_key: ct-a2, id: 702, number: 702, author_login: alice, created_at: "2026-11-01T00:00:00Z", closed_at: "2026-11-01T06:00:00Z", merged_at: "2026-11-01T06:00:00Z", merge_commit_sha: ct-merge-a2} + - {$ref: ../templates/git_activity.yaml#/templates/pull_request, unique_key: ct-a3, id: 703, number: 703, author_login: alice, created_at: "2026-11-01T00:00:00Z", closed_at: "2026-11-01T10:00:00Z", merged_at: "2026-11-01T10:00:00Z", merge_commit_sha: ct-merge-a3} + - {$ref: ../templates/git_activity.yaml#/templates/pull_request, unique_key: ct-a4, id: 704, number: 704, author_login: alice, created_at: "2026-11-01T00:00:00Z", closed_at: "2026-11-02T06:00:00Z", merged_at: "2026-11-02T06:00:00Z", merge_commit_sha: ct-merge-a4} + - {$ref: ../templates/git_activity.yaml#/templates/pull_request, unique_key: ct-a5, id: 705, number: 705, author_login: alice, created_at: "2026-11-01T00:00:00Z", closed_at: "2026-11-05T04:00:00Z", merged_at: "2026-11-05T04:00:00Z", merge_commit_sha: ct-merge-a5} + # dave: closed 2 h in, then reopened and merged 8 h after that. The source + # reports both times and they disagree; the merge is the one that ended the + # cycle, so this request ran 10 h. + - {$ref: ../templates/git_activity.yaml#/templates/pull_request, unique_key: ct-d1, id: 711, number: 711, author_login: dave, created_at: "2026-11-10T00:00:00Z", closed_at: "2026-11-10T02:00:00Z", merged_at: "2026-11-10T10:00:00Z", merge_commit_sha: ct-merge-d1} + # carol: one still open, one closed without ever merging. Neither has a + # cycle, and neither may be treated as merged at its close. + - {$ref: ../templates/git_activity.yaml#/templates/pull_request, unique_key: ct-c1, id: 721, number: 721, state: open, author_login: carol, created_at: "2026-11-18T00:00:00Z"} + - {$ref: ../templates/git_activity.yaml#/templates/pull_request, unique_key: ct-c2, id: 722, number: 722, author_login: carol, created_at: "2026-11-18T01:00:00Z", closed_at: "2026-11-19T00:00:00Z"} + # erin: the source records the merge two hours BEFORE the opening. The + # interval does not run forwards, so there is no cycle to report. + - {$ref: ../templates/git_activity.yaml#/templates/pull_request, unique_key: ct-e1, id: 731, number: 731, author_login: erin, created_at: "2026-11-20T10:00:00Z", closed_at: "2026-11-20T08:00:00Z", merged_at: "2026-11-20T08:00:00Z", merge_commit_sha: ct-merge-e1} + + bronze_github.pull_request_diff_stats: + # GitHub staging reads a request's author address from here and nowhere else. + - {$ref: ../templates/git_activity.yaml#/templates/diff_stat, unique_key: ct-ds-a1, pull_number: 701, additions: 10, author_email: alice@example.com} + - {$ref: ../templates/git_activity.yaml#/templates/diff_stat, unique_key: ct-ds-a2, pull_number: 702, additions: 10, author_email: alice@example.com} + - {$ref: ../templates/git_activity.yaml#/templates/diff_stat, unique_key: ct-ds-a3, pull_number: 703, additions: 10, author_email: alice@example.com} + - {$ref: ../templates/git_activity.yaml#/templates/diff_stat, unique_key: ct-ds-a4, pull_number: 704, additions: 10, author_email: alice@example.com} + - {$ref: ../templates/git_activity.yaml#/templates/diff_stat, unique_key: ct-ds-a5, pull_number: 705, additions: 10, author_email: alice@example.com} + - {$ref: ../templates/git_activity.yaml#/templates/diff_stat, unique_key: ct-ds-d1, pull_number: 711, additions: 10, author_email: dave@example.com} + - {$ref: ../templates/git_activity.yaml#/templates/diff_stat, unique_key: ct-ds-c1, pull_number: 721, additions: 10, author_email: carol@example.com} + - {$ref: ../templates/git_activity.yaml#/templates/diff_stat, unique_key: ct-ds-c2, pull_number: 722, additions: 10, author_email: carol@example.com} + - {$ref: ../templates/git_activity.yaml#/templates/diff_stat, unique_key: ct-ds-e1, pull_number: 731, additions: 10, author_email: erin@example.com} + + bronze_gitlab.repositories: + - {$ref: ../templates/gitlab_git.yaml#/templates/project, unique_key: ct-gl-proj-101} + + bronze_gitlab.users: + - {$ref: ../templates/gitlab_git.yaml#/templates/user, unique_key: ct-gl-user-bob, id: 731, username: bob-gl-cycle, name: Bob Beta} + + bronze_gitlab.pull_requests: + # Merged with no separate closing time: the ordinary GitLab shape, 8 h. + - {$ref: ../templates/gitlab_git.yaml#/templates/merge_request, unique_key: ct-gl-mr-621, iid: 621, id: 96621, author_id: 731, author_username: bob-gl-cycle, created_at: "2026-11-15T00:00:00Z", merged_at: "2026-11-15T08:00:00Z", merge_commit_sha: ct-gl-merge-621} + # Closed an hour in, reopened, merged 9 h in. Both times are reported and + # the merge is the one that ended the cycle. + - {$ref: ../templates/gitlab_git.yaml#/templates/merge_request, unique_key: ct-gl-mr-622, iid: 622, id: 96622, author_id: 731, author_username: bob-gl-cycle, created_at: "2026-11-16T00:00:00Z", closed_at: "2026-11-16T01:00:00Z", merged_at: "2026-11-16T09:00:00Z", merge_commit_sha: ct-gl-merge-622} + + bronze_bitbucket_cloud.commit_authors: + # The one edge that turns heidi's account into a person: the connector + # states the account as a workspace participant, and the seed attaches it + # to whoever this verified address names. Nothing else binds her here. + - {$ref: ../templates/bitbucket_git.yaml#/templates/commit_author, unique_key: "ct-bb-ca:heidi", repo_full_name: acme/cycle, author_email: heidi@example.com, author_account_id: bb-cycle-01, author_uuid: "{bb-cycle-01}", author_nickname: heidi-bb, author_display_name: Heidi Eta} + + bronze_bitbucket_cloud.commits: + # 902's merge commit, collected — the corroboration the recovery needs. It + # carries no author address on purpose: it exists to corroborate a merge, + # and an addressless commit reaches no commit or line metric. + - {$ref: ../templates/bitbucket_git.yaml#/templates/commit, unique_key: "ct:c:902", repository: https://bitbucket.org/acme/cycle.git, sha: aa11bb22cc330000000000000000000000000000, authored_date: "2026-11-26T08:00:00Z", committed_date: "2026-11-26T08:00:00Z", is_merge: true} + + bronze_bitbucket_cloud.pull_requests: + # 901 — its activity holds a terminal merge entry, so the close time is the + # one the source recorded: 12 h. + - {$ref: ../templates/bitbucket_git.yaml#/templates/pull_request, unique_key: "ct-bb-pr:901", repo_full_name: acme/cycle, id: 901, author_display_name: Heidi Eta, author_account_id: bb-cycle-01, created_on: "2026-11-25T00:00:00Z", updated_on: "2026-11-25T14:00:00Z", merge_commit_sha: ct-bb-merge-901} + # 902 — merged by pushing the request's head, so there is no merge ACTION + # and no activity at all. The recovery dates the merge by the request's own + # last update, corroborated by the collected merge commit, so it counts as + # merged — but the source stated no close time, so there is no interval to + # report and it contributes no cycle. + - {$ref: ../templates/bitbucket_git.yaml#/templates/pull_request, unique_key: "ct-bb-pr:902", repo_full_name: acme/cycle, id: 902, author_display_name: Heidi Eta, author_account_id: bb-cycle-01, created_on: "2026-11-26T00:00:00Z", updated_on: "2026-11-26T09:00:00Z", merge_commit_sha: aa11bb22cc33} + + bronze_bitbucket_cloud.pull_request_activity: + - {$ref: ../templates/bitbucket_git.yaml#/templates/activity, unique_key: "ct-bb-ac:901", repo_full_name: acme/cycle, pr_id: 901, update_state: MERGED, event_date: "2026-11-25T12:00:00Z"} diff --git a/tests/datapath/metrics/git/git_pr_size.test.yaml b/tests/datapath/metrics/git/git_pr_size.test.yaml new file mode 100644 index 000000000..8777b17da --- /dev/null +++ b/tests/datapath/metrics/git/git_pr_size.test.yaml @@ -0,0 +1,167 @@ +spec_version: 1 +description: > + Metric: git.pr_size — typical diff size per pull request, #3066. + How it's computed (bronze → silver → gold): + • bronze: pull requests, and per-source diff totals — GitHub reports one + total per request, Bitbucket reports one row per changed FILE and carries + the parent request's last-update stamp on each, GitLab reports one total + per request in a stream of its own that lands only once GitLab has + computed the summary + • silver: one class row per request, whose line counts are null when the + source never reported them and a number when it did + • gold: lines added plus lines removed, one value per request, dated by + the day the request was OPENED — the state it reached is irrelevant, so an + open, a merged and a closed-unmerged request all contribute + • serve: quantileExactInclusive(0.5) over the period's values + + A request whose line counts were never collected contributes nothing. A + request whose counts WERE collected and are zero contributes a real zero: it + is an observed diff of no lines, not an absence, and the class columns are + nullable precisely to keep the two apart. #3362 + + The median is the textbook one — on an even sample both middle values are + averaged, so the answer need not be a size any request had. #3362 + + Bitbucket's per-file rows accumulate: the key is the file path, so a + ReplacingMergeTree replaces a file's row and never removes one. A request + re-collected after a rebase dropped a file therefore holds that file's row + for ever, and only the parent's last-update stamp says which rows belong to + the diff the request has NOW. Size is the newest stamp's rows, whole — taking + each file's own newest row instead keeps the dropped file, since that file has + no newer row to displace it. #3066 + + alice opens five requests sized 12, 24, 36, 48 and 100 lines, so her period + median is 36 and a mean (44) fails it. The 2026-12-01 bucket holds the EVEN + set [12, 24, 36, 48] and serves 30, both middle values averaged; the upper + middle alone would read 36. + carol opens three sized 0, 10 and 30, so her median is 10 while dropping the + observed zero would serve 30. + heidi's Bitbucket requests: one summing three current file rows to 40, one + whose current diff is 30 lines beside a stale 70-line row, and one whose + diffstat was never collected at all — so her period medians [30, 40] to 35, + where summing the stale row would reach 70. + bob opens one GitLab request on each of 12-01 and 12-02, so each day isolates + one of them: 602 has a computed summary and serves 24 lines, 601 has none yet + and serves nothing while still counting as created. Over both days his median + stays 24 — a pending summary read as a zero would halve it to 12. + + dave and erin carry one request each, sized 50 and 60, so the department + cohort is six people deep over 12-01..12-13 — an EVEN peer pool, whose sorted + values are [10, 24, 35, 36, 50, 60]. Its median averages the two middles to + 35.5; the middle order statistic alone would read 36. p25 and p75 stay order + statistics and are not asked to interpolate. #3362 + + heidi is reached the way a real installation reaches her, not by a seeded + binding: her requests name her by account id alone, the connector states that + account as a workspace participant, and the seed attaches it to the person her + collected commit address names (#3423). A Bitbucket assertion here therefore + fails if that path breaks, instead of passing over it. + + Cases: the median across views and the even-bucket boundary; a collected zero + is a value; GitLab serves the summary it has; GitLab stays silent on the one + it has not computed, without dropping the request; Bitbucket sums the current + file rows, ignores a stale one, and stays silent with no rows at all; the peer + view's median over an even cohort; the window holding every close and merge + holds no creation; an empty window. + +identity_accounts: + # GitLab exposes a user's address only to an admin-scoped token, so its + # request reaches a person through the account alone. Bitbucket is NOT bound + # here: it walks the real path — the connector states the account as a + # workspace participant and the seed attaches it to the person its commit + # address names (#3423) — so this spec exercises that path instead of + # standing in for it. + - {source_type: gitlab, source_id: gitlab-test, account_id: "730", person: bob@example.com} + +bronze: + bronze_bamboohr.employees: + - $ref: ../templates/people.yaml#/templates/alice + - $ref: ../templates/people.yaml#/templates/bob + - $ref: ../templates/people.yaml#/templates/carol + - $ref: ../templates/people.yaml#/templates/dave + - $ref: ../templates/people.yaml#/templates/erin + - $ref: ../templates/people.yaml#/templates/heidi + + bronze_github.pull_requests: + # alice: four opened on 12-01, one on 12-02, every close and merge landing + # days later — so a window over the closes must hold nothing. + - {$ref: ../templates/git_activity.yaml#/templates/pull_request, unique_key: ps-a1, id: 501, number: 501, author_login: alice, created_at: "2026-12-01T08:00:00Z", closed_at: "2026-12-05T10:00:00Z", merged_at: "2026-12-05T10:00:00Z", merge_commit_sha: ps-merge-a1} + # Still open: no close, no merge, and it counts all the same. + - {$ref: ../templates/git_activity.yaml#/templates/pull_request, unique_key: ps-a2, id: 502, number: 502, state: open, author_login: alice, created_at: "2026-12-01T09:00:00Z"} + # Closed without ever merging: also counts. + - {$ref: ../templates/git_activity.yaml#/templates/pull_request, unique_key: ps-a3, id: 503, number: 503, author_login: alice, created_at: "2026-12-01T10:00:00Z", closed_at: "2026-12-04T10:00:00Z"} + - {$ref: ../templates/git_activity.yaml#/templates/pull_request, unique_key: ps-a4, id: 504, number: 504, author_login: alice, created_at: "2026-12-01T11:00:00Z", closed_at: "2026-12-06T10:00:00Z", merged_at: "2026-12-06T10:00:00Z", merge_commit_sha: ps-merge-a4} + - {$ref: ../templates/git_activity.yaml#/templates/pull_request, unique_key: ps-a5, id: 505, number: 505, author_login: alice, created_at: "2026-12-02T08:00:00Z", closed_at: "2026-12-07T10:00:00Z", merged_at: "2026-12-07T10:00:00Z", merge_commit_sha: ps-merge-a5} + # carol: the zero-line request sits between her other two, so the median + # moves if it is dropped. + - {$ref: ../templates/git_activity.yaml#/templates/pull_request, unique_key: ps-c1, id: 511, number: 511, author_login: carol, created_at: "2026-12-01T08:00:00Z", closed_at: "2026-12-05T10:00:00Z", merged_at: "2026-12-05T10:00:00Z", merge_commit_sha: ps-merge-c1} + - {$ref: ../templates/git_activity.yaml#/templates/pull_request, unique_key: ps-c2, id: 512, number: 512, author_login: carol, created_at: "2026-12-01T09:00:00Z", closed_at: "2026-12-05T11:00:00Z", merged_at: "2026-12-05T11:00:00Z", merge_commit_sha: ps-merge-c2} + - {$ref: ../templates/git_activity.yaml#/templates/pull_request, unique_key: ps-c3, id: 513, number: 513, author_login: carol, created_at: "2026-12-01T10:00:00Z", closed_at: "2026-12-05T12:00:00Z", merged_at: "2026-12-05T12:00:00Z", merge_commit_sha: ps-merge-c3} + # dave and erin exist to make the department cohort six, so the peer view + # has an EVEN pool and its median has two middles to average. + - {$ref: ../templates/git_activity.yaml#/templates/pull_request, unique_key: ps-d1, id: 521, number: 521, author_login: dave, created_at: "2026-12-03T08:00:00Z", closed_at: "2026-12-05T10:00:00Z", merged_at: "2026-12-05T10:00:00Z", merge_commit_sha: ps-merge-d1} + - {$ref: ../templates/git_activity.yaml#/templates/pull_request, unique_key: ps-e1, id: 531, number: 531, author_login: erin, created_at: "2026-12-03T09:00:00Z", closed_at: "2026-12-05T11:00:00Z", merged_at: "2026-12-05T11:00:00Z", merge_commit_sha: ps-merge-e1} + + bronze_github.pull_request_diff_stats: + # GitHub reports a request's totals in one row, and staging reads the + # author's address from here and nowhere else. + - {$ref: ../templates/git_activity.yaml#/templates/diff_stat, unique_key: ps-ds-a1, pull_number: 501, additions: 10, deletions: 2, author_email: alice@example.com} + - {$ref: ../templates/git_activity.yaml#/templates/diff_stat, unique_key: ps-ds-a2, pull_number: 502, additions: 20, deletions: 4, author_email: alice@example.com} + - {$ref: ../templates/git_activity.yaml#/templates/diff_stat, unique_key: ps-ds-a3, pull_number: 503, additions: 30, deletions: 6, author_email: alice@example.com} + - {$ref: ../templates/git_activity.yaml#/templates/diff_stat, unique_key: ps-ds-a4, pull_number: 504, additions: 40, deletions: 8, author_email: alice@example.com} + - {$ref: ../templates/git_activity.yaml#/templates/diff_stat, unique_key: ps-ds-a5, pull_number: 505, additions: 80, deletions: 20, author_email: alice@example.com} + # Collected, and both counts are zero: an observed diff of no lines. The + # shape a rename-only or mode-only request leaves. + - {$ref: ../templates/git_activity.yaml#/templates/diff_stat, unique_key: ps-ds-c1, pull_number: 511, additions: 0, deletions: 0, author_email: carol@example.com} + - {$ref: ../templates/git_activity.yaml#/templates/diff_stat, unique_key: ps-ds-c2, pull_number: 512, additions: 8, deletions: 2, author_email: carol@example.com} + - {$ref: ../templates/git_activity.yaml#/templates/diff_stat, unique_key: ps-ds-c3, pull_number: 513, additions: 25, deletions: 5, author_email: carol@example.com} + # 50 and 60 sit ABOVE the two middles of the cohort, so they decide its size + # without moving which values the median has to average. + - {$ref: ../templates/git_activity.yaml#/templates/diff_stat, unique_key: ps-ds-d1, pull_number: 521, additions: 40, deletions: 10, author_email: dave@example.com} + - {$ref: ../templates/git_activity.yaml#/templates/diff_stat, unique_key: ps-ds-e1, pull_number: 531, additions: 50, deletions: 10, author_email: erin@example.com} + + bronze_gitlab.repositories: + - {$ref: ../templates/gitlab_git.yaml#/templates/project, unique_key: ps-gl-proj-101} + + bronze_gitlab.users: + - {$ref: ../templates/gitlab_git.yaml#/templates/user, unique_key: ps-gl-user-bob, id: 730, username: bob-gl-size, name: Bob Beta} + + bronze_gitlab.pull_requests: + # 602 — GitLab computed the summary, so this one carries a real size. It + # opens on 12-01 and 601 on 12-02, so each day isolates one of the two. + - {$ref: ../templates/gitlab_git.yaml#/templates/merge_request, unique_key: ps-gl-mr-602, iid: 602, id: 96602, author_id: 730, author_username: bob-gl-size, created_at: "2026-12-01T09:00:00Z", merged_at: "2026-12-06T09:00:00Z", merge_commit_sha: ps-gl-merge-602} + # 601 — no diff-stats row exists for it, so the summary is still pending + # and no line count can reach the class row. + - {$ref: ../templates/gitlab_git.yaml#/templates/merge_request, unique_key: ps-gl-mr-601, iid: 601, id: 96601, author_id: 730, author_username: bob-gl-size, created_at: "2026-12-02T08:00:00Z", merged_at: "2026-12-05T09:00:00Z", merge_commit_sha: ps-gl-merge-601} + + bronze_gitlab.pull_request_diff_stats: + # Only 602. A pending summary has no row at all, which is what keeps 601 + # out of the metric instead of handing it a zero. + - {$ref: ../templates/gitlab_git.yaml#/templates/diff_stats, unique_key: ps-gl-ds-602, mr_iid: 602, additions: 18, deletions: 6, files_changed: 3} + + bronze_bitbucket_cloud.commit_authors: + # The one edge that turns heidi's account into a person: the connector + # states the account as a workspace participant, and the seed attaches it + # to whoever this verified address names. Nothing else binds her here. + - {$ref: ../templates/bitbucket_git.yaml#/templates/commit_author, unique_key: "ps-bb-ca:heidi", repo_full_name: acme/size, author_email: heidi@example.com, author_account_id: bb-size-01, author_uuid: "{bb-size-01}", author_nickname: heidi-bb, author_display_name: Heidi Eta} + + bronze_bitbucket_cloud.pull_requests: + # Left OPEN on purpose: size ignores the state, and an open request cannot + # engage the close-time derivation, so nothing here depends on it. + - {$ref: ../templates/bitbucket_git.yaml#/templates/pull_request, unique_key: "ps-bb-pr:801", repo_full_name: acme/size, id: 801, state: OPEN, author_display_name: Heidi Eta, author_account_id: bb-size-01, created_on: "2026-12-11T08:00:00Z", updated_on: "2026-12-11T09:00:00Z"} + - {$ref: ../templates/bitbucket_git.yaml#/templates/pull_request, unique_key: "ps-bb-pr:802", repo_full_name: acme/size, id: 802, state: OPEN, author_display_name: Heidi Eta, author_account_id: bb-size-01, created_on: "2026-12-12T08:00:00Z", updated_on: "2026-12-12T12:00:00Z"} + # No diffstat row exists for this one: the size was never collected. + - {$ref: ../templates/bitbucket_git.yaml#/templates/pull_request, unique_key: "ps-bb-pr:803", repo_full_name: acme/size, id: 803, state: OPEN, author_display_name: Heidi Eta, author_account_id: bb-size-01, created_on: "2026-12-13T08:00:00Z", updated_on: "2026-12-13T09:00:00Z"} + + bronze_bitbucket_cloud.pull_request_diffstat: + # 801 — three files, all carrying the request's current update stamp: + # 6 + 12 + 22 = 40 lines. + - {$ref: ../templates/bitbucket_git.yaml#/templates/diffstat, unique_key: "ps-bb-ds:801:one", repo_full_name: acme/size, pr_id: 801, file_path: one.py, lines_added: 5, lines_removed: 1, pr_updated_on: "2026-12-11T09:00:00Z"} + - {$ref: ../templates/bitbucket_git.yaml#/templates/diffstat, unique_key: "ps-bb-ds:801:two", repo_full_name: acme/size, pr_id: 801, file_path: two.py, lines_added: 10, lines_removed: 2, pr_updated_on: "2026-12-11T09:00:00Z"} + - {$ref: ../templates/bitbucket_git.yaml#/templates/diffstat, unique_key: "ps-bb-ds:801:three", repo_full_name: acme/size, pr_id: 801, file_path: three.py, lines_added: 20, lines_removed: 2, pr_updated_on: "2026-12-11T09:00:00Z"} + # 802 — the current diff is kept.py alone, 30 lines, collected when the + # request last moved. + - {$ref: ../templates/bitbucket_git.yaml#/templates/diffstat, unique_key: "ps-bb-ds:802:kept", repo_full_name: acme/size, pr_id: 802, file_path: kept.py, lines_added: 25, lines_removed: 5, pr_updated_on: "2026-12-12T12:00:00Z"} + # dropped.py left the diff in a rebase. Its row was written under the + # earlier stamp and nothing replaces or removes it. + - {$ref: ../templates/bitbucket_git.yaml#/templates/diffstat, unique_key: "ps-bb-ds:802:dropped", repo_full_name: acme/size, pr_id: 802, file_path: dropped.py, lines_added: 70, lines_removed: 0, pr_updated_on: "2026-12-12T09:00:00Z"} diff --git a/tests/datapath/metrics/git/git_uncollected_file_changes.test.yaml b/tests/datapath/metrics/git/git_uncollected_file_changes.test.yaml index 50dde7046..542a81431 100644 --- a/tests/datapath/metrics/git/git_uncollected_file_changes.test.yaml +++ b/tests/datapath/metrics/git/git_uncollected_file_changes.test.yaml @@ -13,10 +13,10 @@ description: >- the halves of that rule nothing else exercises. Commit size reads the same corrected numbers: alice's sizes are {11, 22} - (median 22 — quantileExact(0.5) on an even count takes the upper middle), - bob's single empty commit reads 0, and carol's repeat commit keeps only the - part of its own stats the dedup did not remove: 26 reported less the 11 - deduplicated lines is 15, so her median over {11, 15} is 15. + (median 16.5 — an even count averages both middles), bob's single empty + commit reads 0, and carol's repeat commit keeps only the part of its own + stats the dedup did not remove: 26 reported less the 11 deduplicated lines + is 15, so her median over {11, 15} is 13. INVARIANT: dave's commits must stay on the other side of the branch split — no default-branch flag, no merged request listing them, and no change whose diff --git a/tests/datapath/metrics/git/test_git_bitbucket_merge_time_recovery.py b/tests/datapath/metrics/git/test_git_bitbucket_merge_time_recovery.py index d393fa4e7..dc80700a7 100644 --- a/tests/datapath/metrics/git/test_git_bitbucket_merge_time_recovery.py +++ b/tests/datapath/metrics/git/test_git_bitbucket_merge_time_recovery.py @@ -143,15 +143,17 @@ def test_a_merge_with_no_usable_evidence_is_dropped_not_dated_at_the_epoch( r.row("git.non_default_branch_prs_merged", "period", entity_id=HEIDI).equals(value=None) -def test_a_recovered_close_time_is_usable_and_not_merely_countable(spec: SpecRun) -> None: - """A recovered close time has to survive the guards every duration measure applies — - a close before the opening yields no value at all — so the cycle time is what proves - the recovery produced a coherent interval and not just a countable row. - - Of the five requests that landed, the four the recovery dated span 77, 26, 77 and 2 - hours from opening to close; the one the terminal entry dated spans 97. The median of - those five is 77 — a value only reachable if the recovered closes are both present and - ordered after their openings. +def test_a_recovered_close_time_counts_the_merge_but_yields_no_duration( + spec: SpecRun, +) -> None: + """Six requests count as merged (asserted above); exactly one of them has a cycle. + + A recovered close time settles which DAY a merge landed on, and that is all a count + needs. An interval measured to it would be one nobody observed, so the class contract + keeps the SOURCE-stated close apart and every duration measure reads only that. Of + the requests whose opening is usable, the four the recovery dated would have spanned + 77, 26, 77 and 2 hours and contribute nothing; the one a terminal activity entry + dated spans 97, and is the whole of the median. #3362 """ r = spec.call( { @@ -166,7 +168,7 @@ def test_a_recovered_close_time_is_usable_and_not_merely_countable(spec: SpecRun ) assert r.status == 200 - r.row("git.pr_cycle_time_h", "period", entity_id=HEIDI).equals(value=77) + r.row("git.pr_cycle_time_h", "period", entity_id=HEIDI).equals(value=97) def test_a_declined_request_carrying_a_merge_hash_gains_no_close_time(spec: SpecRun) -> None: diff --git a/tests/datapath/metrics/git/test_git_commit_size.py b/tests/datapath/metrics/git/test_git_commit_size.py index 131a398b9..50a41f99b 100644 --- a/tests/datapath/metrics/git/test_git_commit_size.py +++ b/tests/datapath/metrics/git/test_git_commit_size.py @@ -51,7 +51,7 @@ def test_the_period_value_is_the_exact_median_of_the_per_commit_sizes(spec: Spec points={"bucket_start": "2026-10-01", "value": 30} ) r.row("git.commit_size", "timeseries", entity_id=ERIN).contains( - points={"bucket_start": "2026-10-02", "value": 6} + points={"bucket_start": "2026-10-02", "value": 5} ) r.row( "git.commit_size", @@ -64,7 +64,7 @@ def test_the_period_value_is_the_exact_median_of_the_per_commit_sizes(spec: Spec "breakdown", entity_id=ERIN, dimensions={"key": "repository", "value": "git-test:acme/web"}, - ).equals(value=6) + ).equals(value=5) r.row("git.commit_size", "histogram", entity_id=ERIN).contains(bins={"lo": 4, "count": 3}) r.row("git.commit_size", "histogram", entity_id=ERIN).contains(bins={"hi": 204, "count": 1}) diff --git a/tests/datapath/metrics/git/test_git_commit_work_identity.py b/tests/datapath/metrics/git/test_git_commit_work_identity.py index fe680c785..b3a8017fc 100644 --- a/tests/datapath/metrics/git/test_git_commit_work_identity.py +++ b/tests/datapath/metrics/git/test_git_commit_work_identity.py @@ -248,7 +248,7 @@ def test_file_rows_under_the_repository_that_lost_the_collapse_still_count(spec: def test_merge_result_with_partly_collected_branch_commits_stays_counted(spec: SpecRun) -> None: """One of two linked commits was collected, so both commits count; the overlapping - blob folds once (3 + 2 lines) and the sizes {3, 2} read 3.""" + blob folds once (3 + 2 lines) and the sizes {3, 2} median to 2.5.""" r = spec.call( { "url": "/v1/metric-results", @@ -277,7 +277,7 @@ def test_merge_result_with_partly_collected_branch_commits_stays_counted(spec: S by_repository(r, "git.commits", PARTIAL_BRANCH).equals(value=2) by_repository(r, "git.lines_added", PARTIAL_BRANCH).equals(value=5) - by_repository(r, "git.commit_size", PARTIAL_BRANCH).equals(value=3) + by_repository(r, "git.commit_size", PARTIAL_BRANCH).equals(value=2.5) def test_same_patch_id_in_unrelated_repositories_is_two_authored_changes(spec: SpecRun) -> None: diff --git a/tests/datapath/metrics/git/test_git_commits_bitbucket.py b/tests/datapath/metrics/git/test_git_commits_bitbucket.py index c8d7b40db..d24cba7cf 100644 --- a/tests/datapath/metrics/git/test_git_commits_bitbucket.py +++ b/tests/datapath/metrics/git/test_git_commits_bitbucket.py @@ -4,7 +4,7 @@ rows, driven through the bitbucket_cloud staging models into the shared git classes. Line counts come from the file-change rows, so a commit with no file changes contributes zero rather than dropping out; a merge commit is excluded from the commit -count and contributes no size; the even-count size median takes the upper middle. +count and contributes no size; the even-count size median averages both middles. """ from __future__ import annotations @@ -23,7 +23,7 @@ def test_bitbucket_git_metrics_resolve_through_the_source_breakdown(spec: SpecRun) -> None: """commit-a and commit-b count and the merge commit does not; lines come from the - one file-change row; sizes {12, 0} give an upper-middle median of 12.""" + one file-change row; sizes {12, 0} median to 6, both middles averaged.""" r = spec.call( { "url": "/v1/metric-results", @@ -69,7 +69,7 @@ def test_bitbucket_git_metrics_resolve_through_the_source_breakdown(spec: SpecRu value=2 ) r.row("git.commit_size", "breakdown", entity_id=ERIN, dimensions=SOURCE_BITBUCKET).equals( - value=12 + value=6 ) r.row( "git.commits_per_active_day", "breakdown", entity_id=ERIN, dimensions=SOURCE_BITBUCKET diff --git a/tests/datapath/metrics/git/test_git_metrics.py b/tests/datapath/metrics/git/test_git_metrics.py index 0759d932b..1ac5db287 100644 --- a/tests/datapath/metrics/git/test_git_metrics.py +++ b/tests/datapath/metrics/git/test_git_metrics.py @@ -96,7 +96,7 @@ def test_merge_rate_is_zero_when_created_pull_requests_never_merge(spec: SpecRun def test_unified_git_metrics(spec: SpecRun) -> None: - """Erin's day: two commits sized 50 and 60 (the median is the upper middle, 60), one merged + """Erin's day: two commits sized 50 and 60 (the median averages both middles, 55), one merged PR with two reviewers; lines_added counts the fileless second commit, code_lines does not.""" r = spec.call( { @@ -396,14 +396,14 @@ def test_unified_git_metrics(spec: SpecRun) -> None: == commits_per_repository ) - r.row("git.commit_size", "period", entity_id=ERIN).equals(value=60) + r.row("git.commit_size", "period", entity_id=ERIN).equals(value=55) r.row("git.commit_size", "peer", entity_id=ERIN).equals( - target_value=60, p25=24, median=36, p75=48, min=12, max=60, n=5 + target_value=55, p25=24, median=36, p75=48, min=12, max=55, n=5 ) r.row("git.commit_size", "timeseries", entity_id=ERIN).contains( - points={"bucket_start": "2026-10-01", "value": 60} + points={"bucket_start": "2026-10-01", "value": 55} ) - r.row("git.commit_size", "breakdown", entity_id=ERIN, dimensions=SOURCE_GITHUB).equals(value=60) + r.row("git.commit_size", "breakdown", entity_id=ERIN, dimensions=SOURCE_GITHUB).equals(value=55) commit_size_histogram = r.row("git.commit_size", "histogram", entity_id=ERIN) commit_size_histogram.contains(bins={"lo": 50, "count": 1}) commit_size_histogram.contains(bins={"hi": 60, "count": 1}) diff --git a/tests/datapath/metrics/git/test_git_pr_commits.py b/tests/datapath/metrics/git/test_git_pr_commits.py index f1046bd59..4290109fb 100644 --- a/tests/datapath/metrics/git/test_git_pr_commits.py +++ b/tests/datapath/metrics/git/test_git_pr_commits.py @@ -53,7 +53,7 @@ def test_median_commits_per_merged_pull_request(spec: SpecRun) -> None: target_value=2, p25=2, median=3, p75=5, min=1, max=6, n=5 ) r.row("git.pr_commits", "timeseries", entity_id=ALICE).contains( - points={"bucket_start": "2026-10-01", "value": 2} + points={"bucket_start": "2026-10-01", "value": 1.5} ) r.row("git.pr_commits", "timeseries", entity_id=ALICE).contains( points={"bucket_start": "2026-10-02", "value": 4} diff --git a/tests/datapath/metrics/git/test_git_pr_cycle_time.py b/tests/datapath/metrics/git/test_git_pr_cycle_time.py new file mode 100644 index 000000000..91a743323 --- /dev/null +++ b/tests/datapath/metrics/git/test_git_pr_cycle_time.py @@ -0,0 +1,234 @@ +"""Typical hours from opening a pull request to merging it, served per person. + +One value per MERGED request, dated by the merge. A merged request is measured to +its MERGE and never to an earlier close the source reports beside it. On Bitbucket +the merge time is not on the request, so the close is taken from the terminal entry +in its activity where there is one and recovered from the request's own last update +otherwise — the first is a measurement, the second an approximation. +""" + +from __future__ import annotations + +import pytest +from insight_datapath.spec_runner import SpecRun + +pytestmark = pytest.mark.fixture + +SPEC = "git_pr_cycle_time" + +ALICE = "alice@example.com" +BOB = "bob@example.com" +CAROL = "carol@example.com" +DAVE = "dave@example.com" +ERIN = "erin@example.com" +HEIDI = "heidi@example.com" + +SOURCE_GITHUB = {"key": "source", "value": "github"} + + +def test_the_median_cycle_sits_on_the_merge_day(spec: SpecRun) -> None: + """[2, 6, 10, 30, 100] medians to 10, not the mean 29.6. + + All five open at the same instant on 2026-11-01, so every bucket boundary here is + a merge: 2026-11-01 holds [2, 6, 10] and serves 6, and the 100 h request lands on + 2026-11-05 rather than the day it was opened. + """ + r = spec.call( + { + "url": "/v1/metric-results", + "method": "POST", + "body": { + "entity": {"type": "person", "ids": [ALICE]}, + "period": {"from": "2026-11-01", "to": "2026-11-05"}, + "metrics": [ + { + "metric_key": "git.pr_cycle_time_h", + "views": [ + {"view": "period"}, + {"view": "timeseries", "bucket": "day"}, + {"view": "breakdown", "dimensions": ["source"]}, + {"view": "histogram"}, + ], + } + ], + }, + } + ) + assert r.status == 200 + + r.row("git.pr_cycle_time_h", "period", entity_id=ALICE).equals(value=10) + series = r.row("git.pr_cycle_time_h", "timeseries", entity_id=ALICE) + series.contains(points={"bucket_start": "2026-11-01", "value": 6}) + series.contains(points={"bucket_start": "2026-11-02", "value": 30}) + series.contains(points={"bucket_start": "2026-11-05", "value": 100}) + r.row("git.pr_cycle_time_h", "breakdown", entity_id=ALICE, dimensions=SOURCE_GITHUB).equals( + value=10 + ) + r.row("git.pr_cycle_time_h", "histogram", entity_id=ALICE).nonempty("bins") + + +def test_a_github_merge_is_measured_to_the_merge_not_an_earlier_close( + spec: SpecRun, +) -> None: + """dave's request closed 2 h in and merged 10 h in, so its cycle is 10 h. + + The source reports both times. A cycle ends when the change lands, so the merge + decides; taking the close would report an interval that ended while the request + was still open. + """ + r = spec.call( + { + "url": "/v1/metric-results", + "method": "POST", + "body": { + "entity": {"type": "person", "ids": [DAVE]}, + "period": {"from": "2026-11-10", "to": "2026-11-10"}, + "metrics": [{"metric_key": "git.pr_cycle_time_h", "views": [{"view": "period"}]}], + }, + } + ) + assert r.status == 200 + r.row("git.pr_cycle_time_h", "period", entity_id=DAVE).equals(value=10) + + +def test_a_gitlab_merge_is_measured_to_the_merge_not_an_earlier_close( + spec: SpecRun, +) -> None: + """bob's requests run 8 h and 9 h; the second reports a close 1 h in and is not 1. + + GitLab reports a closing time on a request that was closed without merging, and + a request closed, reopened and then merged keeps it. It is not the merge. + """ + r = spec.call( + { + "url": "/v1/metric-results", + "method": "POST", + "body": { + "entity": {"type": "person", "ids": [BOB]}, + "period": {"from": "2026-11-15", "to": "2026-11-16"}, + "metrics": [ + { + "metric_key": "git.pr_cycle_time_h", + "views": [ + {"view": "period"}, + {"view": "timeseries", "bucket": "day"}, + ], + } + ], + }, + } + ) + assert r.status == 200 + + series = r.row("git.pr_cycle_time_h", "timeseries", entity_id=BOB) + series.contains(points={"bucket_start": "2026-11-15", "value": 8}) + series.contains(points={"bucket_start": "2026-11-16", "value": 9}) + r.row("git.pr_cycle_time_h", "period", entity_id=BOB).equals(value=8.5) + + +def test_a_recovered_bitbucket_close_counts_the_merge_but_reports_no_cycle( + spec: SpecRun, +) -> None: + """901 runs 12 h from its terminal activity entry; 902 reports no cycle at all. + + 902 was merged by pushing its head, so its activity holds no terminal entry and + its close time is recovered from the request's own last update. The recovery + settles which DAY the merge landed on, which is why 902 still counts as merged — + but an interval measured to it would be one nobody observed, so the duration is + absent rather than approximate. Both requests merged inside the window, and the + merged count says so. + """ + r = spec.call( + { + "url": "/v1/metric-results", + "method": "POST", + "body": { + "entity": {"type": "person", "ids": [HEIDI]}, + "period": {"from": "2026-11-25", "to": "2026-11-26"}, + "metrics": [ + { + "metric_key": "git.pr_cycle_time_h", + "views": [ + {"view": "period"}, + {"view": "timeseries", "bucket": "day"}, + ], + }, + {"metric_key": "git.prs_merged", "views": [{"view": "period"}]}, + ], + }, + } + ) + assert r.status == 200 + + series = r.row("git.pr_cycle_time_h", "timeseries", entity_id=HEIDI) + series.contains(points={"bucket_start": "2026-11-25", "value": 12}) + series.contains(points={"bucket_start": "2026-11-26", "value": None}) + r.row("git.pr_cycle_time_h", "period", entity_id=HEIDI).equals(value=12) + r.row("git.prs_merged", "period", entity_id=HEIDI).equals(value=2) + + +def test_a_request_that_never_merged_contributes_nothing(spec: SpecRun) -> None: + """carol's open request and her closed-unmerged one have no cycle between them. + + Her created count is asserted beside it: a null cycle must mean "these never + merged", not "carol never opened anything". + """ + r = spec.call( + { + "url": "/v1/metric-results", + "method": "POST", + "body": { + "entity": {"type": "person", "ids": [CAROL]}, + "period": {"from": "2026-11-18", "to": "2026-11-19"}, + "metrics": [ + {"metric_key": "git.pr_cycle_time_h", "views": [{"view": "period"}]}, + {"metric_key": "git.prs_created", "views": [{"view": "period"}]}, + ], + }, + } + ) + assert r.status == 200 + r.row("git.pr_cycle_time_h", "period", entity_id=CAROL).equals(value=None) + r.row("git.prs_created", "period", entity_id=CAROL).equals(value=2) + + +def test_an_interval_that_runs_backwards_is_dropped(spec: SpecRun) -> None: + """erin's merge is recorded before her opening, so there is no cycle at all. + + Not a negative duration and not a zero — the source contradicts itself and the + honest answer is silence. Her created count proves the request itself arrived. + """ + r = spec.call( + { + "url": "/v1/metric-results", + "method": "POST", + "body": { + "entity": {"type": "person", "ids": [ERIN]}, + "period": {"from": "2026-11-01", "to": "2026-11-30"}, + "metrics": [ + {"metric_key": "git.pr_cycle_time_h", "views": [{"view": "period"}]}, + {"metric_key": "git.prs_created", "views": [{"view": "period"}]}, + ], + }, + } + ) + assert r.status == 200 + r.row("git.pr_cycle_time_h", "period", entity_id=ERIN).equals(value=None) + r.row("git.prs_created", "period", entity_id=ERIN).equals(value=1) + + +def test_empty_window(spec: SpecRun) -> None: + """A window with no merged requests serves an honest null, not a zero.""" + r = spec.call( + { + "url": "/v1/metric-results", + "method": "POST", + "body": { + "entity": {"type": "person", "ids": [ALICE]}, + "period": {"from": "2025-01-01", "to": "2025-01-31"}, + "metrics": [{"metric_key": "git.pr_cycle_time_h", "views": [{"view": "period"}]}], + }, + } + ) + assert r.status == 200 + r.row("git.pr_cycle_time_h", "period", entity_id=ALICE).equals(value=None) diff --git a/tests/datapath/metrics/git/test_git_pr_size.py b/tests/datapath/metrics/git/test_git_pr_size.py new file mode 100644 index 000000000..d6ad48ed6 --- /dev/null +++ b/tests/datapath/metrics/git/test_git_pr_size.py @@ -0,0 +1,285 @@ +"""Typical diff size per pull request, served per person over a window. + +Lines added plus lines removed, one value per request, dated by the day the request +was OPENED and taken whatever state the request reached. A request whose line counts +were never collected contributes nothing; a request whose counts were collected and +are zero contributes a real zero. Bitbucket reports one row per changed file, and only +the parent request's last-update stamp says which rows belong to the diff it has now. +""" + +from __future__ import annotations + +import pytest +from insight_datapath.spec_runner import SpecRun + +pytestmark = pytest.mark.fixture + +SPEC = "git_pr_size" + +ALICE = "alice@example.com" +BOB = "bob@example.com" +CAROL = "carol@example.com" +HEIDI = "heidi@example.com" + +#: The whole window every one of the six department members has a value in. +COHORT_PERIOD = {"from": "2026-12-01", "to": "2026-12-13"} + +SOURCE_GITHUB = {"key": "source", "value": "github"} + + +def test_the_median_takes_every_request_whatever_it_became(spec: SpecRun) -> None: + """[12, 24 open, 36 closed-unmerged, 48, 100] medians to 36, not the mean 44. + + The 2026-12-01 bucket holds the even set [12, 24, 36, 48] and serves 30, both + middle values averaged; answering with the upper middle alone would read 36. + """ + r = spec.call( + { + "url": "/v1/metric-results", + "method": "POST", + "body": { + "entity": {"type": "person", "ids": [ALICE]}, + "period": {"from": "2026-12-01", "to": "2026-12-02"}, + "metrics": [ + { + "metric_key": "git.pr_size", + "views": [ + {"view": "period"}, + {"view": "timeseries", "bucket": "day"}, + {"view": "breakdown", "dimensions": ["source"]}, + {"view": "histogram"}, + ], + } + ], + }, + } + ) + assert r.status == 200 + + r.row("git.pr_size", "period", entity_id=ALICE).equals(value=36) + series = r.row("git.pr_size", "timeseries", entity_id=ALICE) + series.contains(points={"bucket_start": "2026-12-01", "value": 30}) + series.contains(points={"bucket_start": "2026-12-02", "value": 100}) + r.row("git.pr_size", "breakdown", entity_id=ALICE, dimensions=SOURCE_GITHUB).equals(value=36) + r.row("git.pr_size", "histogram", entity_id=ALICE).nonempty("bins") + + +def test_a_collected_zero_is_a_value_not_an_absence(spec: SpecRun) -> None: + """carol's [0, 10, 30] medians to 10; dropping the observed zero would serve 30. + + Her zero-line request had its counts collected — the source answered, and the + answer was that no lines changed. That is an observation, and the class columns + are nullable so that a request whose counts were never collected can say so + instead. + """ + r = spec.call( + { + "url": "/v1/metric-results", + "method": "POST", + "body": { + "entity": {"type": "person", "ids": [CAROL]}, + "period": {"from": "2026-12-01", "to": "2026-12-01"}, + "metrics": [{"metric_key": "git.pr_size", "views": [{"view": "period"}]}], + }, + } + ) + assert r.status == 200 + r.row("git.pr_size", "period", entity_id=CAROL).equals(value=10) + + +def test_gitlab_serves_the_diff_summary_its_own_stream_reported(spec: SpecRun) -> None: + """602's summary was computed, so 18 + 6 reaches the metric as 24 on its open day.""" + r = spec.call( + { + "url": "/v1/metric-results", + "method": "POST", + "body": { + "entity": {"type": "person", "ids": [BOB]}, + "period": {"from": "2026-12-01", "to": "2026-12-01"}, + "metrics": [ + {"metric_key": "git.pr_size", "views": [{"view": "period"}]}, + {"metric_key": "git.prs_created", "views": [{"view": "period"}]}, + ], + }, + } + ) + assert r.status == 200 + r.row("git.pr_size", "period", entity_id=BOB).equals(value=24) + r.row("git.prs_created", "period", entity_id=BOB).equals(value=1) + + +def test_gitlab_reports_the_request_whose_summary_is_still_pending_but_no_size( + spec: SpecRun, +) -> None: + """601 has no diff-stats row yet, so its day carries a request and no size. + + The created count is asserted beside the null: without it, a null size would + equally describe a request that never reached the metric at all. The two days + together then median to 24 rather than the 12 a pending summary read as a zero + would give. + """ + r = spec.call( + { + "url": "/v1/metric-results", + "method": "POST", + "body": { + "entity": {"type": "person", "ids": [BOB]}, + "period": {"from": "2026-12-02", "to": "2026-12-02"}, + "metrics": [ + {"metric_key": "git.pr_size", "views": [{"view": "period"}]}, + {"metric_key": "git.prs_created", "views": [{"view": "period"}]}, + ], + }, + } + ) + assert r.status == 200 + r.row("git.pr_size", "period", entity_id=BOB).equals(value=None) + r.row("git.prs_created", "period", entity_id=BOB).equals(value=1) + + both = spec.call( + { + "url": "/v1/metric-results", + "method": "POST", + "body": { + "entity": {"type": "person", "ids": [BOB]}, + "period": {"from": "2026-12-01", "to": "2026-12-02"}, + "metrics": [ + {"metric_key": "git.pr_size", "views": [{"view": "period"}]}, + {"metric_key": "git.prs_created", "views": [{"view": "period"}]}, + ], + }, + } + ) + assert both.status == 200 + both.row("git.pr_size", "period", entity_id=BOB).equals(value=24) + both.row("git.prs_created", "period", entity_id=BOB).equals(value=2) + + +def test_bitbucket_takes_the_current_file_rows_and_not_a_stale_one(spec: SpecRun) -> None: + """801 sums its three current files to 40; 802's diff is 30 lines, not 100. + + 802 holds a 70-line row for a file that left the diff in a rebase, written under + an earlier update stamp. Size is the newest stamp's rows taken whole — resolving + each file to its own newest row would keep the dropped file, which has no newer + row to displace it. The period medians [30, 40] to 35; summing the stale row + would reach 70. + """ + r = spec.call( + { + "url": "/v1/metric-results", + "method": "POST", + "body": { + "entity": {"type": "person", "ids": [HEIDI]}, + "period": {"from": "2026-12-11", "to": "2026-12-13"}, + "metrics": [ + { + "metric_key": "git.pr_size", + "views": [ + {"view": "period"}, + {"view": "timeseries", "bucket": "day"}, + ], + } + ], + }, + } + ) + assert r.status == 200 + + series = r.row("git.pr_size", "timeseries", entity_id=HEIDI) + series.contains(points={"bucket_start": "2026-12-11", "value": 40}) + series.contains(points={"bucket_start": "2026-12-12", "value": 30}) + r.row("git.pr_size", "period", entity_id=HEIDI).equals(value=35) + + +def test_a_bitbucket_request_with_no_diffstat_contributes_nothing(spec: SpecRun) -> None: + """803's size was never collected, so 2026-12-13 carries no size — but a request. + + Bitbucket names the author by account id alone, so 803 reaches the metric through the + binding the connector states for a workspace participant (#3423), and its created + count proves it arrived. The absent size is therefore the missing diffstat and + nothing else. + """ + r = spec.call( + { + "url": "/v1/metric-results", + "method": "POST", + "body": { + "entity": {"type": "person", "ids": [HEIDI]}, + "period": {"from": "2026-12-13", "to": "2026-12-13"}, + "metrics": [ + {"metric_key": "git.pr_size", "views": [{"view": "period"}]}, + {"metric_key": "git.prs_created", "views": [{"view": "period"}]}, + ], + }, + } + ) + assert r.status == 200 + r.row("git.pr_size", "period", entity_id=HEIDI).equals(value=None) + r.row("git.prs_created", "period", entity_id=HEIDI).equals(value=1) + + +def test_the_peer_median_over_an_even_cohort_averages_both_middles(spec: SpecRun) -> None: + """Six members sized [10, 24, 35, 36, 50, 60] disclose 35.5, not the middle 36. + + The peer median answers the same question as the period one, so it reads the same + textbook definition; p25 and p75 stay order statistics over the cohort. + """ + r = spec.call( + { + "url": "/v1/metric-results", + "method": "POST", + "body": { + "entity": {"type": "person", "ids": [ALICE]}, + "period": COHORT_PERIOD, + "metrics": [ + { + "metric_key": "git.pr_size", + "views": [{"view": "period"}, {"view": "peer"}], + } + ], + }, + } + ) + assert r.status == 200 + + # p25/p75 follow ClickHouse quantilesExactIf element selection over the + # 6-member pool; confirmed against a live run. + r.row("git.pr_size", "period", entity_id=ALICE).equals(value=36) + r.row("git.pr_size", "peer", entity_id=ALICE).equals( + target_value=36, p25=24, median=35.5, p75=50, min=10, max=60, n=6 + ) + + +def test_the_window_holding_every_close_holds_no_creation(spec: SpecRun) -> None: + """Every one of alice's requests closes or merges inside 12-04…12-07, and the + window serves nothing: the value sits on the day the request was opened.""" + r = spec.call( + { + "url": "/v1/metric-results", + "method": "POST", + "body": { + "entity": {"type": "person", "ids": [ALICE]}, + "period": {"from": "2026-12-04", "to": "2026-12-07"}, + "metrics": [{"metric_key": "git.pr_size", "views": [{"view": "period"}]}], + }, + } + ) + assert r.status == 200 + r.row("git.pr_size", "period", entity_id=ALICE).equals(value=None) + + +def test_empty_window(spec: SpecRun) -> None: + """A window with no requests serves an honest null, not a zero.""" + r = spec.call( + { + "url": "/v1/metric-results", + "method": "POST", + "body": { + "entity": {"type": "person", "ids": [ALICE]}, + "period": {"from": "2025-01-01", "to": "2025-01-31"}, + "metrics": [{"metric_key": "git.pr_size", "views": [{"view": "period"}]}], + }, + } + ) + assert r.status == 200 + r.row("git.pr_size", "period", entity_id=ALICE).equals(value=None) diff --git a/tests/datapath/metrics/git/test_git_uncollected_file_changes.py b/tests/datapath/metrics/git/test_git_uncollected_file_changes.py index 46a994978..54346b5c0 100644 --- a/tests/datapath/metrics/git/test_git_uncollected_file_changes.py +++ b/tests/datapath/metrics/git/test_git_uncollected_file_changes.py @@ -71,7 +71,7 @@ def test_a_commit_with_no_file_changes_still_reports_its_own_size(spec: SpecRun) ).equals(value=20) r.row("git.code_lines", "period", entity_id=ALICE).equals(value=10) - r.row("git.commit_size", "period", entity_id=ALICE).equals(value=22) + r.row("git.commit_size", "period", entity_id=ALICE).equals(value=16.5) def test_a_commit_that_changed_nothing_reports_zero_not_an_absent_value(spec: SpecRun) -> None: @@ -102,7 +102,7 @@ def test_a_commit_that_changed_nothing_reports_zero_not_an_absent_value(spec: Sp def test_commit_that_lost_the_content_dedup_is_not_treated_as_uncollected(spec: SpecRun) -> None: """carol's repeat commit lost the content dedup, so no `__unknown__` grain appears for it - and Commit size keeps only the 15 lines the dedup did not remove.""" + and keeps only the 15 lines the dedup did not remove, so her sizes {11, 15} median to 13.""" r = spec.call( { "url": "/v1/metric-results", @@ -142,7 +142,7 @@ def test_commit_that_lost_the_content_dedup_is_not_treated_as_uncollected(spec: ) assert unknown_grain == [] - r.row("git.commit_size", "period", entity_id=CAROL).equals(value=15) + r.row("git.commit_size", "period", entity_id=CAROL).equals(value=13) def test_an_uncollected_size_on_a_branch_reaches_the_lines_but_not_the_code_lines( diff --git a/tests/datapath/metrics/schemas/bronze_bitbucket_cloud.pull_request_diffstat.yaml b/tests/datapath/metrics/schemas/bronze_bitbucket_cloud.pull_request_diffstat.yaml index 4f438ef18..27e3e658b 100644 --- a/tests/datapath/metrics/schemas/bronze_bitbucket_cloud.pull_request_diffstat.yaml +++ b/tests/datapath/metrics/schemas/bronze_bitbucket_cloud.pull_request_diffstat.yaml @@ -19,6 +19,7 @@ schemas: status: { type: [string, "null"] } lines_added: { type: [integer, "null"] } lines_removed: { type: [integer, "null"] } + pr_updated_on: { type: [string, "null"] } _airbyte_raw_id: { type: string } _airbyte_extracted_at: { type: string, format: date-time } _airbyte_meta: { type: string } diff --git a/tests/datapath/metrics/schemas/bronze_github.pull_requests.yaml b/tests/datapath/metrics/schemas/bronze_github.pull_requests.yaml index ce82cc719..3783acc69 100644 --- a/tests/datapath/metrics/schemas/bronze_github.pull_requests.yaml +++ b/tests/datapath/metrics/schemas/bronze_github.pull_requests.yaml @@ -22,6 +22,6 @@ schemas: base_ref: {type: string} created_at: {type: string} updated_at: {type: string} - closed_at: {type: string} - merged_at: {type: string} - merge_commit_sha: {type: string} + closed_at: {type: [string, "null"]} + merged_at: {type: [string, "null"]} + merge_commit_sha: {type: [string, "null"]} diff --git a/tests/datapath/metrics/templates/bitbucket_git.yaml b/tests/datapath/metrics/templates/bitbucket_git.yaml index ef9c0f6c5..c6c2ba3c0 100644 --- a/tests/datapath/metrics/templates/bitbucket_git.yaml +++ b/tests/datapath/metrics/templates/bitbucket_git.yaml @@ -30,6 +30,21 @@ templates: changed_files: 0 is_in_default_branch: true + # The address an account has committed under. A pull request names its author + # by account id alone, so this row is the edge the persons-seed walks to reach + # the person the roster already knows — see #3423. + commit_author: + $ref: "#/templates/base" + repo_full_name: acme/platform + collected_at: "2026-10-02T00:00:00Z" + author_email: null + author_account_id: null + author_uuid: null + author_nickname: null + author_display_name: null + sample_sha: "0000000000000000000000000000000000000000" + last_committed_date: "2026-10-01T09:00:00Z" + file_change: $ref: "#/templates/base" repository: https://bitbucket.org/acme/platform.git diff --git a/tests/datapath/metrics/templates/gitlab_git.yaml b/tests/datapath/metrics/templates/gitlab_git.yaml index 350866d18..84abe60e1 100644 --- a/tests/datapath/metrics/templates/gitlab_git.yaml +++ b/tests/datapath/metrics/templates/gitlab_git.yaml @@ -125,6 +125,18 @@ templates: detailed_merge_status: not_open user_notes_count: 0 + # The merge request's diff summary, its own stream. A request GitLab has not + # computed the summary for yet has no row here at all. + diff_stats: + $ref: "#/templates/base" + project_id: 101 + repo_path: acme/platform + mr_iid: null + updated_at: "2026-10-01T13:00:00.000+00:00" + additions: 0 + deletions: 0 + files_changed: 0 + # A merge-request note. `system: true` rows are GitLab's own log of the # request; the review verdicts among them are recognised by body. note: diff --git a/tests/stand/api/analytics/test_drilldown.py b/tests/stand/api/analytics/test_drilldown.py index 750496355..bd1d94a4b 100644 --- a/tests/stand/api/analytics/test_drilldown.py +++ b/tests/stand/api/analytics/test_drilldown.py @@ -592,17 +592,15 @@ def _assert_shape(walk: _Walk, expectation: Expectation, person_id: str) -> None def _assert_median(period: float | None, values: Sequence[float], metric_key: str) -> None: - """`quantileExact(0.5)` returns a stored element, so this is an identity. + """The median of the evidence, both middle values averaged on an even count. - Both middle elements are accepted rather than one: which of them the server - returns for an even count is its own tie rule, and pinning it here would test - ClickHouse rather than the evidence. + An exact identity, not a membership test: the server serves + `quantileExactInclusive(0.5)`, which is the textbook median, so there is no + tie rule left to guess at. #3362 """ - ordered = sorted(values) - middle = {ordered[(len(ordered) - 1) // 2], ordered[len(ordered) // 2]} - assert period in middle, ( - f"{metric_key}: period {period} is neither middle value of {len(ordered)} " - f"evidence rows {sorted(middle)}" + expected = statistics.median(values) + assert period is not None and period == pytest.approx(expected), ( + f"{metric_key}: period {period} is not the median {expected} of {len(values)} evidence rows" ) @@ -611,10 +609,11 @@ def _assert_percentile( ) -> None: """`quantileExact(p)` returns a stored element, so this is an identity too. - The element sits at index `floor(p x n)` of the sorted values. Its - neighbour below is accepted as well, for the same reason `_assert_median` - accepts both middle values: which side of a tie the server takes is its own - rule, and pinning it here would test ClickHouse rather than the evidence. + The element sits at index `floor(p x n)` of the sorted values, and its + neighbour below is accepted too: which side of a tie the server takes is its + own rule, and pinning it here would test ClickHouse rather than the + evidence. A percentile keeps `quantileExact` where the median does not — + unlike the median it has no second definition to match. #3362 """ ordered = sorted(values) index = min(len(ordered) - 1, int(quantile * len(ordered)))