Minor test cleanups. #624

ggreer · 2026-01-07T22:57:01Z

Check error in test.
Use t.Context() instead of context.Background().
Log sync in sync correctness test.
Skip race condition test on Windows since its filesystem is very slow, causing CI to time out.

Summary by CodeRabbit

Tests
- Enhanced error handling in benchmark tests to validate file operations.
- Improved test context initialization for more reliable timeout propagation.
- Modernized test loops for clearer, more consistent test behavior.
Chores
- CI updated to run platform-specific test steps and produce machine-readable test output for more consistent, fresh test runs.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

coderabbitai · 2026-01-07T22:57:24Z

Walkthrough

Small test and CI adjustments: benchmark now checks the error from closing a temp file; correctness test initializes an SDK logger and reuses that contextual ctx for timeouts; CI workflow adds a Windows-specific Go test step alongside the existing non-Windows step.

Changes

Cohort / File(s)	Summary
Benchmark error handling `pkg/sync/expand/expand_benchmark_test.go`	`tmpFile.Close()` is now assigned to `err` and its error is checked with `require.NoError` instead of being ignored.
Contextual logger & context propagation `pkg/sync/expand/expand_correctness_test.go`	Imports `sdkLogging`, initializes a contextual logger via `sdkLogging.Init` at test start, and reuses the returned `ctx` for timeout context creation in `TestExpandCorrectness`.
Loop style updates in tests `pkg/dotc1z/race_test.go`	Replaced index-based `for` loops with `range`-based iterations for several nested loops (e.g., `for ... range 50`).
CI workflow (Windows test step added) `.github/workflows/ci.yaml`	Added a Windows-specific `go test` step (runs with `-short`, outputs JSON to `test.json`) while leaving the existing non-Windows test step unchanged.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Possibly related PRs

A couple of Grant Expansion Benchmarks #594 — Touches the same benchmark helpers and temp-file handling referenced in the benchmark change.
Expansion Source Tests #595 — Modifies TestExpandCorrectness and logger/context behavior similar to this PR.
Better logging of cache config. #223 — Related CI workflow edits affecting Go test steps across platforms.

Suggested reviewers

kans

Poem

🐰 I hopped through tests with gentle care,
Closed a file, tuned logs in the air,
Context carried where timeouts dwell,
Windows got a step to tell,
Small hops, big tidy flair.

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check	❓ Inconclusive	The title 'Minor test cleanups' is vague and generic, using non-descriptive language that doesn't convey meaningful information about the specific changes (error checking, context usage, logging, CI modifications).	Consider a more specific title like 'Test cleanups: error checking, context propagation, logging, and Windows CI' to better summarize the key changes.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

📝 Generate docstrings

📜 Recent review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ec65801 and 1e080a9.

📒 Files selected for processing (4)

.github/workflows/ci.yaml
pkg/dotc1z/race_test.go
pkg/sync/expand/expand_benchmark_test.go
pkg/sync/expand/expand_correctness_test.go

🚧 Files skipped from review as they are similar to previous changes (3)

pkg/sync/expand/expand_correctness_test.go
.github/workflows/ci.yaml
pkg/sync/expand/expand_benchmark_test.go

🧰 Additional context used

🧠 Learnings (3)

📓 Common learnings

Learnt from: ggreer
Repo: ConductorOne/baton-sdk PR: 616
File: pkg/synccompactor/compactor_test.go:44-52
Timestamp: 2026-01-02T17:21:10.051Z
Learning: In tests that specifically validate cleanup behavior (e.g., tests that check whether temp directories are empty after compaction), cleanup failures should fail the test using require.NoError(t, err) and similar assertions, as the cleanup functionality itself is what's being tested.

Learnt from: mchavez
Repo: ConductorOne/baton-sdk PR: 211
File: pkg/uhttp/dbcache.go:0-0
Timestamp: 2024-09-03T15:49:24.881Z
Learning: The user mchavez has fixed the issue related to improving error messages and adding comments in the `cleanup` method of the `DBCache` struct in the `pkg/uhttp/dbcache.go` file.

Learnt from: mchavez
Repo: ConductorOne/baton-sdk PR: 211
File: pkg/uhttp/dbcache.go:0-0
Timestamp: 2024-10-08T21:29:30.695Z
Learning: The user mchavez has fixed the issue related to improving error messages and adding comments in the `cleanup` method of the `DBCache` struct in the `pkg/uhttp/dbcache.go` file.

📚 Learning: 2025-03-10T17:07:23.395Z

Learnt from: ggreer
Repo: ConductorOne/baton-sdk PR: 321
File: pkg/tasks/c1api/service_client.go:190-190
Timestamp: 2025-03-10T17:07:23.395Z
Learning: Go 1.22 (released February 2024) added support for range over integers, allowing syntax like `for i := range n` which iterates from 0 to n-1 and is equivalent to `for i := 0; i < n; i++`.

Applied to files:

pkg/dotc1z/race_test.go

📚 Learning: 2026-01-02T17:21:01.723Z

Learnt from: ggreer
Repo: ConductorOne/baton-sdk PR: 616
File: pkg/synccompactor/compactor_test.go:44-52
Timestamp: 2026-01-02T17:21:01.723Z
Learning: In tests that verify cleanup behavior (e.g., ensuring temporary artifacts are removed or directories are empty after an operation), treat cleanup failures as test failures by asserting on the cleanup call (e.g., require.NoError(t, err) or assert.NoError(t, err)). This ensures that the cleanup path is explicitly tested and any cleanup error fails the test, confirming correct behavior of the cleanup code.

Applied to files:

pkg/dotc1z/race_test.go

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)

GitHub Check: go-test (1.25.2, ubuntu-latest)
GitHub Check: go-test (1.25.2, windows-latest)

🔇 Additional comments (1)

pkg/dotc1z/race_test.go (1)

39-39: LGTM! Clean refactoring to Go 1.22 range-over-integer syntax.

The conversion of all index-based loops to for i := range n syntax is correct and makes the code more concise. All conversions are semantically equivalent to their traditional counterparts.

Also applies to: 52-52, 62-62, 100-100, 124-124, 137-137, 143-143

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In @pkg/sync/expand/expand_correctness_test.go:
- Line 101: The code calls context.WithTimeout(ctx, 5*time.Minute) but ctx is
not defined; replace the undefined parent with a valid context (e.g., use
context.Background() or a test context) so the call becomes ctx, cancel :=
context.WithTimeout(context.Background(), 5*time.Minute) or, if there is an
existing parent context variable in the surrounding test, use that parent name
instead; ensure you still defer cancel() as appropriate.

📜 Review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 66d79f3 and 9268c9c.

📒 Files selected for processing (2)

pkg/sync/expand/expand_benchmark_test.go
pkg/sync/expand/expand_correctness_test.go

🧰 Additional context used

🧠 Learnings (2)

📓 Common learnings

Learnt from: ggreer
Repo: ConductorOne/baton-sdk PR: 616
File: pkg/synccompactor/compactor_test.go:44-52
Timestamp: 2026-01-02T17:21:10.051Z
Learning: In tests that specifically validate cleanup behavior (e.g., tests that check whether temp directories are empty after compaction), cleanup failures should fail the test using require.NoError(t, err) and similar assertions, as the cleanup functionality itself is what's being tested.

📚 Learning: 2026-01-02T17:21:01.723Z

Learnt from: ggreer
Repo: ConductorOne/baton-sdk PR: 616
File: pkg/synccompactor/compactor_test.go:44-52
Timestamp: 2026-01-02T17:21:01.723Z
Learning: In tests that verify cleanup behavior (e.g., ensuring temporary artifacts are removed or directories are empty after an operation), treat cleanup failures as test failures by asserting on the cleanup call (e.g., require.NoError(t, err) or assert.NoError(t, err)). This ensures that the cleanup path is explicitly tested and any cleanup error fails the test, confirming correct behavior of the cleanup code.

Applied to files:

pkg/sync/expand/expand_benchmark_test.go
pkg/sync/expand/expand_correctness_test.go

🪛 GitHub Check: go-lint

pkg/sync/expand/expand_correctness_test.go

[failure] 101-101:
undefined: ctx (typecheck)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)

GitHub Check: go-test (1.25.2, ubuntu-latest)
GitHub Check: go-test (1.25.2, windows-latest)

🔇 Additional comments (1)

pkg/sync/expand/expand_benchmark_test.go (1)

173-174: LGTM: Proper error handling for file close operation.

Capturing and checking the Close() error is good practice and prevents potential silent failures in the benchmark setup.

pkg/sync/expand/expand_correctness_test.go

- Check error in test. - Use t.Context() instead of context.Background(). - Log sync in sync correctness test. - Skip race condition test on Windows since its filesystem is very slow, causing CI to time out.

coderabbitai bot reviewed Jan 7, 2026

View reviewed changes

pkg/sync/expand/expand_correctness_test.go Show resolved Hide resolved

ggreer force-pushed the ggreer/cleanup branch from 9268c9c to ec65801 Compare January 7, 2026 23:17

ggreer changed the title ~~Minor test cleanups: Check error in test. Use same context instead of context.Background().~~ Minor test cleanups. Jan 7, 2026

Minor test cleanups.

1e080a9

- Check error in test. - Use t.Context() instead of context.Background(). - Log sync in sync correctness test. - Skip race condition test on Windows since its filesystem is very slow, causing CI to time out.

ggreer force-pushed the ggreer/cleanup branch from ec65801 to 1e080a9 Compare January 8, 2026 00:35

jugonzalez12 self-requested a review January 8, 2026 00:47

jugonzalez12 approved these changes Jan 8, 2026

View reviewed changes

ggreer merged commit ff60071 into main Jan 8, 2026
6 checks passed

ggreer deleted the ggreer/cleanup branch January 8, 2026 00:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Minor test cleanups. #624

Minor test cleanups. #624

Uh oh!

ggreer commented Jan 7, 2026 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Jan 7, 2026 •

edited

Loading

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Minor test cleanups. #624

Minor test cleanups. #624

Uh oh!

Conversation

ggreer commented Jan 7, 2026 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Jan 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Suggested reviewers

Poem

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ggreer commented Jan 7, 2026 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Jan 7, 2026 •

edited

Loading