test(joiner): add synctest to joiner tests #5265
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors test cases in pkg/file/joiner/joiner_test.go to use Go 1.24+'s testing/synctest package for better time-based test control and deterministic timing. The changes update four test functions to wrap their logic in synctest.Test() calls and replace context.Background() with t.Context() throughout the test file. Additionally, the Makefile has been temporarily modified to run only the joiner tests during CI.
- Wrapped four test functions (
TestJoinerReadAt,TestJoinerOneLevel,TestJoinerTwoLevelsAcrossChunk,TestJoinerIterateChunkAddresses) insynctest.Test() - Replaced
context.Background()witht.Context()across all modified tests - Refactored
TestJoinerRedundancyMultilevelto extract a helper function and usesynctest.Test() - Temporarily limited CI test scope to
./pkg/file/joinerin Makefile
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| pkg/file/joiner/joiner_test.go | Wrapped selected test functions in synctest.Test() and replaced context.Background() with t.Context() for improved time control and test determinism |
| Makefile | Temporarily narrowed CI test scope from ./... to ./pkg/file/joiner to focus on joiner package tests |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| if c != swarm.ChunkSize { | ||
| t.Fatalf("chunk %d expected read %d bytes; got %d", i, swarm.ChunkSize, c) | ||
| if c != 42 { |
There was a problem hiding this comment.
It showed that I changed the line to 42, but in reality it didn’t change at all.
acud
left a comment
acud
left a comment
There was a problem hiding this comment.
It would really be easier to review and approve if the change would be just about adding the synctest helpers. I'm not sure if all the other changes are a result of some vscode auto-linter/formatter or a refactor extension, in which case I would disable it at least for these sort of changes. Mind you that these are delicate components. Every tiny change to tests and production code have to be thoroughly understood before being approved.
akrem-chabchoub
force-pushed
the
migrate-to-synctest-joiner
branch
from
9f89af3 to
8aa1b5f
Compare
Yes, I completely agree. GitHub Pull Requests: SemanticDiff: |
akrem-chabchoub
force-pushed
the
migrate-to-synctest-joiner
branch
from
50ab377 to
a75c2ba
Compare
pkg/file/joiner/joiner_test.go
Outdated
| }) | ||
|
|
||
| // First sanity check and recover a range | ||
| t.Log("Starting test sequence") |
There was a problem hiding this comment.
Is this log intentional or it was used for debugging?
There was a problem hiding this comment.
Its used for debugging, I removed it
| }) | ||
| // Helper function to test reading with different strategies | ||
| canReadRange := func(s getter.Strategy, fallback bool, canRead bool, description string) { | ||
| t.Logf("Testing: %s", description) |
There was a problem hiding this comment.
Is this log intentional or it was used for debugging?
There was a problem hiding this comment.
Yes its intentioanl.
Previously we were using t.Run() and passing the description in it. But since synctest doesn't allow using t.Run() inside it so I passed the description to the canReadRange function.
Checklist
Description
Add synctest in joiner tests.
Open API Spec Version Changes (if applicable)
Motivation and Context (Optional)
Related Issue (Optional)
#5232
Screenshots (if appropriate):