refactor: tests to use testing package helpers instead of manual context handling#891
Open
Not-Dhananjay-Mishra wants to merge 16 commits into
Open
refactor: tests to use testing package helpers instead of manual context handling#891Not-Dhananjay-Mishra wants to merge 16 commits into
testing package helpers instead of manual context handling#891Not-Dhananjay-Mishra wants to merge 16 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughTests were changed to derive contexts and temporary resources from the testing framework (e.g., Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/clock/clock_test.go (1)
137-137: Optional cleanup: remove redundantos.Removedefers.On Line 137, Line 155, Line 178, Line 243, and Line 299, file-level cleanup is now redundant because
t.TempDir()will remove the directory and all created files.♻️ Suggested simplification
- defer func() { _ = os.Remove(tmpFile.Name()) }()Apply this deletion at each listed occurrence.
Also applies to: 155-155, 178-178, 243-243, 299-299
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/clock/clock_test.go` at line 137, Remove the redundant defer calls that call os.Remove(tmpFile.Name()) in the tests: where tmpFile is created under t.TempDir() (the five occurrences that currently use defer func() { _ = os.Remove(tmpFile.Name()) }()). Since t.TempDir() already cleans up the directory and files, delete those defer statements (references to tmpFile and the os.Remove defer) to simplify the tests.gtfsdb/sqlite_performance_test.go (1)
66-67:t.TempDir()cleanup is redundant with explicitos.RemoveAll.Since
t.TempDir()already handles cleanup, these extra defers can be removed to simplify the tests.♻️ Suggested cleanup
func TestFileDatabaseConnectionPool(t *testing.T) { // Create temporary directory for test database tmpDir := t.TempDir() - defer func() { _ = os.RemoveAll(tmpDir) }() dbPath := filepath.Join(tmpDir, "test.db") ... } func TestConnectionPoolBehaviorWithFileDatabase(t *testing.T) { // Create temporary directory for test database tmpDir := t.TempDir() - defer func() { _ = os.RemoveAll(tmpDir) }() dbPath := filepath.Join(tmpDir, "test_concurrent.db") ... }Also applies to: 95-96
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gtfsdb/sqlite_performance_test.go` around lines 66 - 67, The test creates tmpDir with t.TempDir() and then redundantly defers os.RemoveAll(tmpDir); remove the explicit defer that calls os.RemoveAll for the tmpDir created by t.TempDir() (i.e., delete the defer func() { _ = os.RemoveAll(tmpDir) }() lines that follow tmpDir := t.TempDir()), and do the same for the other occurrence around the second tmpDir usage so cleanup relies on t.TempDir() only.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/gtfs/advanced_direction_calculator_test.go`:
- Around line 35-36: The inline comment above the InitGTFSManager call is stale
(mentions context.Background())—update it to reference the actual context source
t.Context() used here; i.e., change the comment that precedes the
InitGTFSManager(t.Context(), gtfsConfig) invocation to state that we pass
t.Context() to satisfy the new cancellable startup logic so it matches the call
involving InitGTFSManager, sharedManager and gtfsConfig.
---
Nitpick comments:
In `@gtfsdb/sqlite_performance_test.go`:
- Around line 66-67: The test creates tmpDir with t.TempDir() and then
redundantly defers os.RemoveAll(tmpDir); remove the explicit defer that calls
os.RemoveAll for the tmpDir created by t.TempDir() (i.e., delete the defer
func() { _ = os.RemoveAll(tmpDir) }() lines that follow tmpDir := t.TempDir()),
and do the same for the other occurrence around the second tmpDir usage so
cleanup relies on t.TempDir() only.
In `@internal/clock/clock_test.go`:
- Line 137: Remove the redundant defer calls that call os.Remove(tmpFile.Name())
in the tests: where tmpFile is created under t.TempDir() (the five occurrences
that currently use defer func() { _ = os.Remove(tmpFile.Name()) }()). Since
t.TempDir() already cleans up the directory and files, delete those defer
statements (references to tmpFile and the os.Remove defer) to simplify the
tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 44e1c1f9-2090-4a17-9fb3-648a6533e9dc
📒 Files selected for processing (51)
cmd/api/app_test.gocmd/api/lock_safety_test.gogtfsdb/bulk_insert_frequency_test.gogtfsdb/bulk_insert_test.gogtfsdb/conditional_import_test.gogtfsdb/connection_pool_test.gogtfsdb/fts_queries_test.gogtfsdb/helpers_test.gogtfsdb/nil_shape_test.gogtfsdb/query_latency_test.gogtfsdb/sqlite_performance_test.gogtfsdb/synthetic_gtfs_frequency_test.gointernal/appconf/config_test.gointernal/appconf/json_config_test.gointernal/clock/clock_test.gointernal/gtfs/advanced_direction_calculator_test.gointernal/gtfs/agency_filter_test.gointernal/gtfs/gtfs_manager_test.gointernal/gtfs/hot_swap_test.gointernal/gtfs/multi_feed_test.gointernal/gtfs/parallel_realtime_test.gointernal/gtfs/realtime_test.gointernal/gtfs/route_search_test.gointernal/gtfs/shape_cache_test.gointernal/gtfs/shapes_test.gointernal/gtfs/shutdown_test.gointernal/gtfs/tidy_test.gointernal/logging/structured_logging_test.gointernal/metrics/metrics_test.gointernal/restapi/arrival_and_departure_for_stop_handler_test.gointernal/restapi/arrivals_and_departures_for_stop_handler_test.gointernal/restapi/block_handler_test.gointernal/restapi/calculate_block_trip_sequence_test.gointernal/restapi/context_cancellation_test.gointernal/restapi/http_test.gointernal/restapi/id_helpers_test.gointernal/restapi/input_validation_integration_test.gointernal/restapi/openapi_conformance_test.gointernal/restapi/route_handler_test.gointernal/restapi/servicedate_timezone_regression_test.gointernal/restapi/shapes_handler_test.gointernal/restapi/stop_handler_test.gointernal/restapi/stops_for_route_handler_test.gointernal/restapi/trip_for_vehicle_handler_test.gointernal/restapi/trip_handler_test.gointernal/restapi/trips_helper_test.gointernal/restapi/vehicles_for_agency_handler_test.gointernal/restapi/vehicles_helper_test.gointernal/utils/filters_test.gointernal/webui/static_handler_test.gointernal/webui/test_helper_test.go
Contributor
Author
|
@fletcherw If you allow me, I can add |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/clock/clock_test.go (1)
131-146: Optional: extract repeated temp-file setup into a test helper.The same
CreateTemp + WriteString + Closesetup appears in multiple tests. A small helper (e.g.,writeTempClockFile(t, content) string) would reduce duplication and make these cases easier to maintain.Also applies to: 148-163, 165-186, 236-250, 292-309
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/clock/clock_test.go` around lines 131 - 146, Extract the repeated temp-file creation logic into a helper (e.g., writeTempClockFile(t testing.TB, content string) string) that calls os.CreateTemp, writes content, closes the file and returns the path; then update TestEnvironmentClock_FromFile and the other tests (the ones around lines 148-163, 165-186, 236-250, 292-309) to call writeTempClockFile(t, expectedTime.Format(time.RFC3339)) and pass the returned filename into NewEnvironmentClock instead of duplicating CreateTemp+WriteString+Close in each test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/clock/clock_test.go`:
- Around line 131-146: Extract the repeated temp-file creation logic into a
helper (e.g., writeTempClockFile(t testing.TB, content string) string) that
calls os.CreateTemp, writes content, closes the file and returns the path; then
update TestEnvironmentClock_FromFile and the other tests (the ones around lines
148-163, 165-186, 236-250, 292-309) to call writeTempClockFile(t,
expectedTime.Format(time.RFC3339)) and pass the returned filename into
NewEnvironmentClock instead of duplicating CreateTemp+WriteString+Close in each
test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 792aeb86-3cf4-431b-8c20-ba66f0f4ed0a
📒 Files selected for processing (2)
gtfsdb/sqlite_performance_test.gointernal/clock/clock_test.go
Not-Dhananjay-Mishra
changed the title
testing package helpers instead of manual context handlingtesting package helpers instead of manual context handling
…InBounds test Co-authored-by: Copilot <copilot@github.com>
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refactor tests to use built-in
testing.Thelpers for context, environment variables, and temporary resources. This removes manual handling and makes tests safer and more isolated.Changes
t.Context()os.Setenv()witht.Setenv()os.CreateTemp("", ...)withos.CreateTemp(t.TempDir(), ...)os.MkdirTemp()witht.TempDir()os.TempDir()witht.TempDir()Summary by CodeRabbit