From da6ecce76ab157a2cdb0c100179a1d811b34df49 Mon Sep 17 00:00:00 2001 From: Julia Shtal Date: Mon, 14 Sep 2026 22:18:37 +0200 Subject: [PATCH] fix(metrics)!: exclude the upper bound from Instant metric windows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The fourteen Instant-windowed queries filtered `between :from and :to`, but every caller passes an exclusive bound — toDate + 1 at midnight UTC. A record stamped on that instant was counted, and for the daily metrics bucketed into a calendar day outside the requested range, writing a snapshot with no coverage row. Half-open windows match every metric document and the convention GitHubPrReviewRepository already applied. --- .../repository/GitCommitEntityRepository.java | 21 ++-- .../GitHubPullRequestRepository.java | 18 +++- .../devanalytics/issue/IssueRepository.java | 12 ++- .../metrics/IssueAttributionQueryTest.java | 49 +++++++++- .../metrics/PrAttributionQueryTest.java | 96 +++++++++++++++++-- .../metrics/StatsCoverageQueryTest.java | 54 +++++++++-- .../calc/CommitAttributionQueryTest.java | 72 +++++++++++++- dev-analytics/tasks/lessons.md | 1 + 8 files changed, 292 insertions(+), 31 deletions(-) diff --git a/dev-analytics/src/main/java/com/juliashtal/devanalytics/git/repository/GitCommitEntityRepository.java b/dev-analytics/src/main/java/com/juliashtal/devanalytics/git/repository/GitCommitEntityRepository.java index 48628f2..f64c1a4 100644 --- a/dev-analytics/src/main/java/com/juliashtal/devanalytics/git/repository/GitCommitEntityRepository.java +++ b/dev-analytics/src/main/java/com/juliashtal/devanalytics/git/repository/GitCommitEntityRepository.java @@ -58,6 +58,9 @@ and c.message like concat('%#', :prNumber, '%') // The bot filter is needed because of that email branch: an automation account configured // with the user's own address satisfies it. No null guard is needed on author_name; the // column is NOT NULL. + // + // Windows are half-open: `from` inclusive, `to` exclusive, so adjacent windows neither + // double-count a record nor drop one. Callers pass toDate + 1 at midnight UTC. // ------------------------------------------------------------------------- @Query(""" @@ -70,7 +73,8 @@ select date(c.authorDate) as day, where r.id IN :repoIds and (c.authorGithubId = :githubUserId or lower(c.authorEmail) in :emails) and c.authorName not like '%[bot]%' - and c.authorDate between :from and :to + and c.authorDate >= :from + and c.authorDate < :to group by date(c.authorDate), r.id order by day, repoId """) @@ -91,7 +95,8 @@ select date(c.authorDate) as day, where r.id IN :repoIds and (c.authorGithubId = :githubUserId or lower(c.authorEmail) in :emails) and c.authorName not like '%[bot]%' - and c.authorDate between :from and :to + and c.authorDate >= :from + and c.authorDate < :to group by date(c.authorDate), r.id order by day, repoId """) @@ -129,7 +134,8 @@ List findCommitsForPrByRepoIds( where c.repository.id IN :repoIds and (c.authorGithubId = :githubUserId or lower(c.authorEmail) in :emails) and c.authorName not like '%[bot]%' - and c.authorDate between :from and :to + and c.authorDate >= :from + and c.authorDate < :to """) List findCommitDetailsByRepoIdsAndIdentity( @Param("repoIds") List repoIds, @@ -148,7 +154,8 @@ List findCommitDetailsByRepoIdsAndIdentity( from GitCommitEntity c where c.repository.id IN :repoIds and c.authorName not like '%[bot]%' - and c.authorDate between :from and :to + and c.authorDate >= :from + and c.authorDate < :to group by c.repository.id """) List countTotalCommitsByRepoIds( @@ -167,7 +174,8 @@ List countTotalCommitsByRepoIds( where c.repository.id IN :repoIds and (c.authorGithubId = :githubUserId or lower(c.authorEmail) in :emails) and c.authorName not like '%[bot]%' - and c.authorDate between :from and :to + and c.authorDate >= :from + and c.authorDate < :to group by c.repository.id """) List countCommitsByRepoIdsAndIdentity( @@ -228,7 +236,8 @@ List findPendingCommitsForRepository( count(c) as recordCount from GitCommitEntity c where c.repository.id in :repoIds - and c.authorDate between :from and :to + and c.authorDate >= :from + and c.authorDate < :to group by c.statsStatus, c.statsSkipReason """) List countByStatsStateInRange( diff --git a/dev-analytics/src/main/java/com/juliashtal/devanalytics/github/repository/GitHubPullRequestRepository.java b/dev-analytics/src/main/java/com/juliashtal/devanalytics/github/repository/GitHubPullRequestRepository.java index 9970162..f12dce3 100644 --- a/dev-analytics/src/main/java/com/juliashtal/devanalytics/github/repository/GitHubPullRequestRepository.java +++ b/dev-analytics/src/main/java/com/juliashtal/devanalytics/github/repository/GitHubPullRequestRepository.java @@ -37,6 +37,9 @@ public interface GitHubPullRequestRepository extends JpaRepository= :from + and p.createdAt < :to group by date(p.createdAt), p.repository.id order by day, repoId """) @@ -64,7 +68,8 @@ select date(p.mergedAt) as day, where p.repository.id IN :repoIds and p.authorGithubId = :githubUserId and p.merged = true - and p.mergedAt between :from and :to + and p.mergedAt >= :from + and p.mergedAt < :to group by date(p.mergedAt), p.repository.id order by day, repoId """) @@ -82,7 +87,8 @@ List aggregatePrMergedDailyByRepoIdsAndAuthorGithubId( where p.repository.id IN :repoIds and p.authorGithubId = :githubUserId and p.merged = true - and p.mergedAt between :from and :to + and p.mergedAt >= :from + and p.mergedAt < :to """) List findMergedLeadTimesByRepoIdsAndAuthorGithubId( @Param("repoIds") List repoIds, @@ -96,7 +102,8 @@ List findMergedLeadTimesByRepoIdsAndAuthorGithubId( where p.repository.id IN :repoIds and p.authorGithubId = :githubUserId and p.merged = true - and p.mergedAt between :from and :to + and p.mergedAt >= :from + and p.mergedAt < :to """) List findMergedPrsByRepoIdsAndAuthorGithubId( @Param("repoIds") List repoIds, @@ -156,7 +163,8 @@ List findPendingPrsForRepository( count(p) as recordCount from GitHubPullRequestEntity p where p.repository.id in :repoIds - and p.createdAt between :from and :to + and p.createdAt >= :from + and p.createdAt < :to group by p.statsStatus, p.statsSkipReason """) List countByStatsStateInRange( diff --git a/dev-analytics/src/main/java/com/juliashtal/devanalytics/issue/IssueRepository.java b/dev-analytics/src/main/java/com/juliashtal/devanalytics/issue/IssueRepository.java index 7cf1d4f..8e276ce 100644 --- a/dev-analytics/src/main/java/com/juliashtal/devanalytics/issue/IssueRepository.java +++ b/dev-analytics/src/main/java/com/juliashtal/devanalytics/issue/IssueRepository.java @@ -43,6 +43,9 @@ public interface IssueRepository extends JpaRepository { // Native SQL: COALESCE(i.repository_id, rm.repository_id) routes GitHub and Jira issues // through one repo-scoped filter, which JPQL cannot express across nullable FKs. Nullable // bind parameters are CAST explicitly, since PostgreSQL cannot type a bare NULL. + // + // Windows are half-open: `from` inclusive, `to` exclusive, so adjacent windows neither + // double-count a record nor drop one. Callers pass toDate + 1 at midnight UTC. // ------------------------------------------------------------------------- @Query(value = """ @@ -54,7 +57,8 @@ SELECT date(i.created_at) AS day, WHERE COALESCE(i.repository_id, rm.repository_id) IN (:repoIds) AND ( (i.source = 'GITHUB' AND i.creator_github_id = CAST(:githubUserId AS bigint)) OR (i.source = 'JIRA' AND i.reporter_account_id = CAST(:jiraAccountId AS text)) ) - AND i.created_at BETWEEN :from AND :to + AND i.created_at >= :from + AND i.created_at < :to GROUP BY date(i.created_at), COALESCE(i.repository_id, rm.repository_id) ORDER BY day, repoId """, nativeQuery = true) @@ -75,7 +79,8 @@ WHERE COALESCE(i.repository_id, rm.repository_id) IN (:repoIds) AND ( (i.source = 'GITHUB' AND i.assignee_github_id = CAST(:githubUserId AS bigint)) OR (i.source = 'JIRA' AND i.assignee_account_id = CAST(:jiraAccountId AS text)) ) AND i.closed_at IS NOT NULL - AND i.closed_at BETWEEN :from AND :to + AND i.closed_at >= :from + AND i.closed_at < :to GROUP BY date(i.closed_at), COALESCE(i.repository_id, rm.repository_id) ORDER BY day, repoId """, nativeQuery = true) @@ -97,7 +102,8 @@ WHERE COALESCE(i.repository_id, rm.repository_id) IN (:repoIds) OR (i.source = 'JIRA' AND i.assignee_account_id = CAST(:jiraAccountId AS text)) ) AND i.created_at IS NOT NULL AND i.closed_at IS NOT NULL - AND i.closed_at BETWEEN :from AND :to + AND i.closed_at >= :from + AND i.closed_at < :to """, nativeQuery = true) List findIssueLeadTimesByRepoIdsAndIdentity( @Param("repoIds") List repoIds, diff --git a/dev-analytics/src/test/java/com/juliashtal/devanalytics/metrics/IssueAttributionQueryTest.java b/dev-analytics/src/test/java/com/juliashtal/devanalytics/metrics/IssueAttributionQueryTest.java index 1957e84..5607882 100644 --- a/dev-analytics/src/test/java/com/juliashtal/devanalytics/metrics/IssueAttributionQueryTest.java +++ b/dev-analytics/src/test/java/com/juliashtal/devanalytics/metrics/IssueAttributionQueryTest.java @@ -110,6 +110,47 @@ void aggregateIssuesClosedDailyByRepoIdsAndIdentity_summedOverTeamMembers_equals assertThat(teamAggregate).isEqualTo(distinctClosedIssuesInRepo()); } + // ------------------------------------------------------------------------- + // Window boundary. `to` is exclusive, so an issue stamped exactly on it belongs to the next + // window. One boundary issue — created by A, assigned to B — covers all three queries. + // ------------------------------------------------------------------------- + + @Test + void aggregateIssuesCreatedDailyByRepoIdsAndIdentity_recordAtWindowEnd_excluded() { + long inside = created(A_GITHUB_ID, A_JIRA); + assertThat(inside).isPositive(); + + insertGithubIssue("GH-AT-WINDOW-END", A_GITHUB_ID, B_GITHUB_ID, TO, TO); + + assertThat(created(A_GITHUB_ID, A_JIRA)).isEqualTo(inside); + } + + @Test + void aggregateIssuesClosedDailyByRepoIdsAndIdentity_recordAtWindowEnd_excluded() { + long inside = closed(B_GITHUB_ID, B_JIRA); + assertThat(inside).isPositive(); + + insertGithubIssue("GH-AT-WINDOW-END", A_GITHUB_ID, B_GITHUB_ID, TO, TO); + + assertThat(closed(B_GITHUB_ID, B_JIRA)).isEqualTo(inside); + } + + @Test + void findIssueLeadTimesByRepoIdsAndIdentity_recordAtWindowEnd_excluded() { + long inside = leadTimeRows(B_GITHUB_ID, B_JIRA); + assertThat(inside).isPositive(); + + insertGithubIssue("GH-AT-WINDOW-END", A_GITHUB_ID, B_GITHUB_ID, TO, TO); + + assertThat(leadTimeRows(B_GITHUB_ID, B_JIRA)).isEqualTo(inside); + } + + /** Lead-time rows the identity matches, one per closed issue. */ + private long leadTimeRows(Long githubUserId, String jiraAccountId) { + return issueRepository.findIssueLeadTimesByRepoIdsAndIdentity( + List.of(repoId), githubUserId, jiraAccountId, FROM, TO).size(); + } + private long created(Long githubUserId, String jiraAccountId) { return sum(issueRepository.aggregateIssuesCreatedDailyByRepoIdsAndIdentity( List.of(repoId), githubUserId, jiraAccountId, FROM, TO)); @@ -163,12 +204,18 @@ private Long insertRepo(Long dataSourceId, String name) { } private void insertGithubIssue(String key, Long creatorGithubId, Long assigneeGithubId) { + insertGithubIssue(key, creatorGithubId, assigneeGithubId, CREATED, CLOSED); + } + + /** Overload for the window cases, where created_at and closed_at are the discriminators. */ + private void insertGithubIssue(String key, Long creatorGithubId, Long assigneeGithubId, + Instant createdAt, Instant closedAt) { jdbc.update( "INSERT INTO issues (repository_id, source_issue_key, title, state, created_at, " + "closed_at, creator_github_id, assignee_github_id, source) " + "VALUES (?, ?, 'Issue', 'closed', ?, ?, ?, ?, 'GITHUB')", repoId, key + "-" + System.nanoTime(), - Timestamp.from(CREATED), Timestamp.from(CLOSED), creatorGithubId, assigneeGithubId); + Timestamp.from(createdAt), Timestamp.from(closedAt), creatorGithubId, assigneeGithubId); } private Long insertJiraDataSource() { diff --git a/dev-analytics/src/test/java/com/juliashtal/devanalytics/metrics/PrAttributionQueryTest.java b/dev-analytics/src/test/java/com/juliashtal/devanalytics/metrics/PrAttributionQueryTest.java index 6cbf8e8..e482d49 100644 --- a/dev-analytics/src/test/java/com/juliashtal/devanalytics/metrics/PrAttributionQueryTest.java +++ b/dev-analytics/src/test/java/com/juliashtal/devanalytics/metrics/PrAttributionQueryTest.java @@ -10,9 +10,8 @@ import org.springframework.boot.test.autoconfigure.orm.jpa.DataJpaTest; import org.springframework.jdbc.core.JdbcTemplate; +import java.sql.Timestamp; import java.time.Instant; -import java.time.LocalDateTime; -import java.time.ZoneOffset; import java.util.List; import static org.assertj.core.api.Assertions.assertThat; @@ -22,8 +21,8 @@ * *

Attribution compares numeric account ids, so a rename cannot split one person's history and * differing spellings cannot defeat the self-review exclusion. The timestamp columns are - * {@code TIMESTAMP WITHOUT TIME ZONE}, so fixtures bind UTC {@link LocalDateTime} — see - * {@code EarliestActivityQueryTest}.

+ * {@code TIMESTAMPTZ}, so fixtures bind an absolute {@link java.sql.Timestamp} rather than a wall + * clock the session zone would reinterpret.

*/ @DataJpaTest @AutoConfigureTestDatabase(replace = AutoConfigureTestDatabase.Replace.NONE) @@ -111,6 +110,76 @@ void countDistinctPrsReviewedByUser_mixOfSelfAndPeerReviews_countsOnlyThePeerRev .isEqualTo(1); } + // ------------------------------------------------------------------------- + // Window boundary. `to` is exclusive, so a PR stamped exactly on it belongs to the next + // window. The merged queries need their own in-window row: seed() merges nothing. + // ------------------------------------------------------------------------- + + @Test + void aggregatePrCreatedDailyByRepoIdsAndAuthorGithubId_recordAtWindowEnd_excluded() { + long inside = prsCreated(); + assertThat(inside).isPositive(); + + insertPr(10, STORED_LOGIN, AUTHOR_ID, TO); + + assertThat(prsCreated()).isEqualTo(inside); + } + + @Test + void aggregatePrMergedDailyByRepoIdsAndAuthorGithubId_recordAtWindowEnd_excluded() { + insertMergedPr(11, AUTHOR_ID, WHEN, WHEN); + long inside = prsMerged(); + assertThat(inside).isPositive(); + + insertMergedPr(12, AUTHOR_ID, WHEN, TO); + + assertThat(prsMerged()).isEqualTo(inside); + } + + @Test + void findMergedLeadTimesByRepoIdsAndAuthorGithubId_recordAtWindowEnd_excluded() { + insertMergedPr(11, AUTHOR_ID, WHEN, WHEN); + long inside = mergedLeadTimeRows(); + assertThat(inside).isPositive(); + + insertMergedPr(12, AUTHOR_ID, WHEN, TO); + + assertThat(mergedLeadTimeRows()).isEqualTo(inside); + } + + @Test + void findMergedPrsByRepoIdsAndAuthorGithubId_recordAtWindowEnd_excluded() { + insertMergedPr(11, AUTHOR_ID, WHEN, WHEN); + int inside = prRepository + .findMergedPrsByRepoIdsAndAuthorGithubId(List.of(repoId), AUTHOR_ID, FROM, TO).size(); + assertThat(inside).isPositive(); + + insertMergedPr(12, AUTHOR_ID, WHEN, TO); + + assertThat(prRepository.findMergedPrsByRepoIdsAndAuthorGithubId(List.of(repoId), AUTHOR_ID, FROM, TO)) + .hasSize(inside); + } + + /** PRs the author created inside the window, summed across the per-day rows. */ + private long prsCreated() { + return prRepository.aggregatePrCreatedDailyByRepoIdsAndAuthorGithubId( + List.of(repoId), AUTHOR_ID, FROM, TO) + .stream().mapToLong(DailyCountProjection::getCount).sum(); + } + + /** PRs the author merged inside the window, summed across the per-day rows. */ + private long prsMerged() { + return prRepository.aggregatePrMergedDailyByRepoIdsAndAuthorGithubId( + List.of(repoId), AUTHOR_ID, FROM, TO) + .stream().mapToLong(DailyCountProjection::getCount).sum(); + } + + /** Lead-time rows for the author's merged PRs inside the window. */ + private long mergedLeadTimeRows() { + return prRepository.findMergedLeadTimesByRepoIdsAndAuthorGithubId( + List.of(repoId), AUTHOR_ID, FROM, TO).size(); + } + // ------------------------------------------------------------------------- // Fixtures // ------------------------------------------------------------------------- @@ -139,12 +208,27 @@ private Long insertRepo(Long dataSourceId, String name) { } private Long insertPr(int number, String authorLogin, Long authorGithubId) { + return insertPr(number, authorLogin, authorGithubId, WHEN); + } + + /** Overload for the window cases, where created_at is the discriminating column. */ + private Long insertPr(int number, String authorLogin, Long authorGithubId, Instant createdAt) { return jdbc.queryForObject( "INSERT INTO github_pull_requests (repository_id, number, title, author_login, " + "author_github_id, state, merged, created_at) " + "VALUES (?, ?, 'PR', ?, ?, 'closed', false, ?) RETURNING id", Long.class, repoId, number, authorLogin, authorGithubId, - LocalDateTime.ofInstant(WHEN, ZoneOffset.UTC)); + Timestamp.from(createdAt)); + } + + /** Merged PRs: the three lead-time and merge queries all filter on merged = true. */ + private Long insertMergedPr(int number, Long authorGithubId, Instant createdAt, Instant mergedAt) { + return jdbc.queryForObject( + "INSERT INTO github_pull_requests (repository_id, number, title, author_login, " + + "author_github_id, state, merged, created_at, merged_at) " + + "VALUES (?, ?, 'PR', ?, ?, 'closed', true, ?, ?) RETURNING id", + Long.class, repoId, number, STORED_LOGIN, authorGithubId, + Timestamp.from(createdAt), Timestamp.from(mergedAt)); } private void insertReview(Long prId, String reviewerLogin, Long reviewerGithubId) { @@ -152,6 +236,6 @@ private void insertReview(Long prId, String reviewerLogin, Long reviewerGithubId "INSERT INTO github_pr_reviews (pr_id, reviewer_login, reviewer_github_id, state, submitted_at) " + "VALUES (?, ?, ?, 'APPROVED', ?)", prId, reviewerLogin, reviewerGithubId, - LocalDateTime.ofInstant(WHEN, ZoneOffset.UTC)); + Timestamp.from(WHEN)); } } diff --git a/dev-analytics/src/test/java/com/juliashtal/devanalytics/metrics/StatsCoverageQueryTest.java b/dev-analytics/src/test/java/com/juliashtal/devanalytics/metrics/StatsCoverageQueryTest.java index 30699aa..74b3945 100644 --- a/dev-analytics/src/test/java/com/juliashtal/devanalytics/metrics/StatsCoverageQueryTest.java +++ b/dev-analytics/src/test/java/com/juliashtal/devanalytics/metrics/StatsCoverageQueryTest.java @@ -12,9 +12,8 @@ import org.springframework.boot.test.autoconfigure.orm.jpa.DataJpaTest; import org.springframework.jdbc.core.JdbcTemplate; +import java.sql.Timestamp; import java.time.Instant; -import java.time.LocalDateTime; -import java.time.ZoneOffset; import java.util.List; import static org.assertj.core.api.Assertions.assertThat; @@ -121,6 +120,32 @@ void countByStatsStateInRange_pullRequests_areSplitByReason() { assertThat(rows.stream().mapToLong(StatsCoverageProjection::getRecordCount).sum()).isEqualTo(3); } + // ------------------------------------------------------------------------- + // Window boundary. StatsCoverageService builds the same exclusive bound as MetricsService, so + // the coverage counts must describe the same population the metrics were computed over. + // ------------------------------------------------------------------------- + + @Test + void countByStatsStateInRange_commitAtWindowEnd_excluded() { + long inside = commitRecordsInRange(); + assertThat(inside).isEqualTo(7); + + insertCommit(TO, "COMPLETE", null); + + assertThat(commitRecordsInRange()).isEqualTo(inside); + } + + @Test + void countByStatsStateInRange_pullRequestAtWindowEnd_excluded() { + insertPullRequest(WHEN, "COMPLETE", null); + long inside = prRecordsInRange(); + assertThat(inside).isPositive(); + + insertPullRequest(TO, "COMPLETE", null); + + assertThat(prRecordsInRange()).isEqualTo(inside); + } + @Test void skipReasonIndex_isUsedForTheSkippedBreakdown() { // Pins the partial index as reachable, not merely present: a sequential scan here would @@ -140,6 +165,18 @@ void skipReasonIndex_isUsedForTheSkippedBreakdown() { assertThat(plan).contains("ix_git_commits_skip_reason"); } + /** Every commit record the coverage query sees in the window, across all groups. */ + private long commitRecordsInRange() { + return commitRepository.countByStatsStateInRange(List.of(repoId), FROM, TO) + .stream().mapToLong(StatsCoverageProjection::getRecordCount).sum(); + } + + /** Every PR record the coverage query sees in the window, across all groups. */ + private long prRecordsInRange() { + return pullRequestRepository.countByStatsStateInRange(List.of(repoId), FROM, TO) + .stream().mapToLong(StatsCoverageProjection::getRecordCount).sum(); + } + private long countOf(List rows, StatsStatus status, StatsSkipReason reason) { return rows.stream() .filter(r -> r.getStatsStatus() == status && r.getStatsSkipReason() == reason) @@ -149,8 +186,8 @@ private long countOf(List rows, StatsStatus status, Sta // ------------------------------------------------------------------------- // Fixtures. Inserted with JdbcTemplate so the test states the exact column values the - // queries read. author_date and created_at are TIMESTAMP WITHOUT TIME ZONE, so Instant - // fixtures are bound as UTC LocalDateTime rather than through the JVM zone. + // queries read. author_date and created_at are TIMESTAMPTZ, so Instant fixtures bind as an + // absolute Timestamp rather than a wall clock the session zone would reinterpret. // ------------------------------------------------------------------------- private void insertCommit(Instant when, String statsStatus, String skipReason) { @@ -160,11 +197,16 @@ private void insertCommit(Instant when, String statsStatus, String skipReason) { + "VALUES (?, ?, 'Fixture', 'fixture@x.org', ?, 'msg', 5, 5, ?, ?)", repoId, "coverage-" + System.nanoTime() + "-" + (++seq), - LocalDateTime.ofInstant(when, ZoneOffset.UTC), + Timestamp.from(when), statsStatus, skipReason); } + /** Allocates its own PR number, for tests that do not care which one it gets. */ + private void insertPullRequest(Instant when, String statsStatus, String skipReason) { + insertPullRequest(100 + (++seq), when, statsStatus, skipReason); + } + private void insertPullRequest(int number, Instant when, String statsStatus, String skipReason) { jdbc.update( "INSERT INTO github_pull_requests (repository_id, number, title, state, merged, " @@ -172,7 +214,7 @@ private void insertPullRequest(int number, Instant when, String statsStatus, Str + "VALUES (?, ?, 'Fixture PR', 'closed', false, ?, ?, ?)", repoId, number, - LocalDateTime.ofInstant(when, ZoneOffset.UTC), + Timestamp.from(when), statsStatus, skipReason); } diff --git a/dev-analytics/src/test/java/com/juliashtal/devanalytics/metrics/calc/CommitAttributionQueryTest.java b/dev-analytics/src/test/java/com/juliashtal/devanalytics/metrics/calc/CommitAttributionQueryTest.java index 3883c00..b056375 100644 --- a/dev-analytics/src/test/java/com/juliashtal/devanalytics/metrics/calc/CommitAttributionQueryTest.java +++ b/dev-analytics/src/test/java/com/juliashtal/devanalytics/metrics/calc/CommitAttributionQueryTest.java @@ -10,9 +10,8 @@ import org.springframework.boot.test.autoconfigure.orm.jpa.DataJpaTest; import org.springframework.jdbc.core.JdbcTemplate; +import java.sql.Timestamp; import java.time.Instant; -import java.time.LocalDateTime; -import java.time.ZoneOffset; import java.util.List; import java.util.Set; @@ -168,6 +167,61 @@ void countTotalCommitsByRepoIds_botCommits_areExcludedFromTheDenominator() { assertThat(siloDenominator()).isEqualTo(5); } + // ------------------------------------------------------------------------- + // Window boundary. `to` is exclusive: MetricsService passes toDate + 1 at midnight UTC, so a + // commit stamped exactly on that instant belongs to the next window, not this one. + // ------------------------------------------------------------------------- + + @Test + void aggregateCommitsDailyByRepoIdsAndIdentity_recordAtWindowEnd_excluded() { + long inside = commitsFor(A_GITHUB_ID, Set.of(A_EMAIL)); + assertThat(inside).isPositive(); + + insertCommit("c-at-window-end", A_EMAIL, A_GITHUB_ID, TO); + + assertThat(commitsFor(A_GITHUB_ID, Set.of(A_EMAIL))).isEqualTo(inside); + } + + @Test + void aggregateChurnDailyByRepoIdsAndIdentity_recordAtWindowEnd_excluded() { + long inside = churnFor(A_GITHUB_ID, Set.of(A_EMAIL)); + assertThat(inside).isPositive(); + + insertCommit("c-at-window-end", A_EMAIL, A_GITHUB_ID, TO); + + assertThat(churnFor(A_GITHUB_ID, Set.of(A_EMAIL))).isEqualTo(inside); + } + + @Test + void findCommitDetailsByRepoIdsAndIdentity_recordAtWindowEnd_excluded() { + long inside = detailsFor(A_GITHUB_ID, Set.of(A_EMAIL)); + assertThat(inside).isPositive(); + + insertCommit("c-at-window-end", A_EMAIL, A_GITHUB_ID, TO); + + assertThat(detailsFor(A_GITHUB_ID, Set.of(A_EMAIL))).isEqualTo(inside); + } + + @Test + void countCommitsByRepoIdsAndIdentity_recordAtWindowEnd_excluded() { + long inside = siloNumeratorFor(A_GITHUB_ID, Set.of(A_EMAIL)); + assertThat(inside).isPositive(); + + insertCommit("c-at-window-end", A_EMAIL, A_GITHUB_ID, TO); + + assertThat(siloNumeratorFor(A_GITHUB_ID, Set.of(A_EMAIL))).isEqualTo(inside); + } + + @Test + void countTotalCommitsByRepoIds_recordAtWindowEnd_excluded() { + long inside = siloDenominator(); + assertThat(inside).isPositive(); + + insertCommit("c-at-window-end", A_EMAIL, A_GITHUB_ID, TO); + + assertThat(siloDenominator()).isEqualTo(inside); + } + /** Total commits the identity matches, summed across the per-day rows the projection returns. */ private long commitsFor(Long githubUserId, Set emails) { List rows = commitRepository.aggregateCommitsDailyByRepoIdsAndIdentity( @@ -236,11 +290,21 @@ private Long insertRepo(Long dataSourceId, String name) { /** Hashes are globally unique, so each carries the test's nanotime suffix. */ private void insertCommit(String hashPrefix, String authorEmail, Long authorGithubId) { - insertCommit(hashPrefix, "Fixture", authorEmail, authorGithubId); + insertCommit(hashPrefix, "Fixture", authorEmail, authorGithubId, WHEN); } /** Overload for the bot cases, where author_name is the discriminating column. */ private void insertCommit(String hashPrefix, String authorName, String authorEmail, Long authorGithubId) { + insertCommit(hashPrefix, authorName, authorEmail, authorGithubId, WHEN); + } + + /** Overload for the window cases, where author_date is the discriminating column. */ + private void insertCommit(String hashPrefix, String authorEmail, Long authorGithubId, Instant when) { + insertCommit(hashPrefix, "Fixture", authorEmail, authorGithubId, when); + } + + private void insertCommit(String hashPrefix, String authorName, String authorEmail, + Long authorGithubId, Instant when) { jdbc.update( "INSERT INTO git_commits (repository_id, hash, author_name, author_email, " + "author_github_id, author_date, message, additions, deletions, stats_status) " @@ -250,6 +314,6 @@ private void insertCommit(String hashPrefix, String authorName, String authorEma authorName, authorEmail, authorGithubId, - LocalDateTime.ofInstant(WHEN, ZoneOffset.UTC)); + Timestamp.from(when)); } } diff --git a/dev-analytics/tasks/lessons.md b/dev-analytics/tasks/lessons.md index 4986f5f..e09b562 100644 --- a/dev-analytics/tasks/lessons.md +++ b/dev-analytics/tasks/lessons.md @@ -32,6 +32,7 @@ Pre-seeded with the mistakes most likely on this codebase, based on architecture - `[seed]` Filtered commits by `User.githubLogin` instead of `User.email` → commits attribute via email; only PRs/reviews attribute via githubLogin. (×0) - `[seed]` New metric calc forgot the `team` parameter and saved with `team = null` always → personal-only metrics break team-scope queries silently. (×0) - `[seed]` Computed a ratio without guarding zero denominator → return null (not 0) when sample size is 0, so the AI layer's `anomaly` detection ignores it. (×0) +- Wrote a windowed query with `between :from and :to` while the caller passed an exclusive upper bound → `BETWEEN` is inclusive at both ends, so an exclusive `to` must be compared with `<`. Fourteen queries drifted from the half-open rule every `docs/metrics` document states and `GitHubPrReviewRepository` already followed; the daily ones bucketed a boundary record into a calendar day outside the requested range and wrote a snapshot with no `metric_coverage` row. Also: fixtures binding `LocalDateTime` into a `TIMESTAMPTZ` column are reinterpreted in the session zone, so a boundary-case test must bind `Timestamp.from(instant)`. Fixed on `118-half-open-metric-windows`, 2026-09-14. (×1) ### Ingestion & async