Skip to content

[test-improver] Improve tests for server shutdown - #4852

Merged
lpcox merged 2 commits into
mainfrom
test-improver/shutdown-test-improvements-80981fad6765b4ae
Apr 30, 2026
Merged

[test-improver] Improve tests for server shutdown#4852
lpcox merged 2 commits into
mainfrom
test-improver/shutdown-test-improvements-80981fad6765b4ae

Conversation

@github-actions

Copy link
Copy Markdown
Contributor

Test Improvements: shutdown_test.go

File Analyzed

  • Test File: internal/server/shutdown_test.go
  • Package: internal/server

Improvements Made

1. Better Testify Assertions

Replaced manual if/else type-assertion blocks with idiomatic testify assertions:

Before:

if errorMsg, ok := response["error"].(string); ok {
    assert.Equal(t, tt.expectError, errorMsg, "Unexpected error message")
} else {
    t.Errorf("Expected error field in response")
}

After:

require.Contains(t, response, "error", "response should contain an 'error' field")
assert.Equal(t, tt.expectError, response["error"], "Unexpected error message")
  • require.Contains now fails fast with a clear message if "error" key is missing
  • assert.Equal directly compares the value without the awkward type assertion
  • ✅ Eliminates use of t.Errorf in favour of testify's consistent assertion API

2. Parallel Test Execution

  • ✅ Added t.Parallel() to TestShutdownBehavior_RoutedMode and TestShutdownBehavior_UnifiedMode
    These tests are self-contained and create their own server/handler per subtest, so they are safe to run in parallel.

Test Execution

All tests pass:

--- PASS: TestShutdownBehavior_RoutedMode (0.01s)
    --- PASS: TestShutdownBehavior_RoutedMode/MCP_endpoint_rejected_with_503_after_shutdown
    --- PASS: TestShutdownBehavior_RoutedMode/MCP_endpoint_with_trailing_slash_rejected_with_503
    --- PASS: TestShutdownBehavior_RoutedMode/Health_endpoint_still_works_during_shutdown
    --- PASS: TestShutdownBehavior_RoutedMode/Close_endpoint_returns_410_on_subsequent_calls
PASS
ok  github.com/github/gh-aw-mcpg/internal/server  0.021s

Why These Changes?

The shutdown_test.go file was using raw t.Errorf in a few places where testify's require.Contains + assert.Equal provides clearer failure messages and better integrates with the rest of the testify-based test suite. The t.Parallel() additions allow the test runner to execute these independent table-driven tests concurrently, reducing total test time.


Generated by Test Improver Workflow
Focuses on better patterns, increased coverage, and more stable tests

Generated by Test Improver · ● 4.6M ·

- Replace manual t.Errorf('Expected error field in response') with idiomatic
  testify assertions using require.Contains and assert.Equal
- Add t.Parallel() to top-level test functions for better test isolation
  and parallelism

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@lpcox
lpcox marked this pull request as ready for review April 30, 2026 13:30
Copilot AI review requested due to automatic review settings April 30, 2026 13:30

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refines the shutdown behavior tests in internal/server by improving assertion patterns and attempting to speed up execution via parallelism.

Changes:

  • Adds t.Parallel() to the routed and unified shutdown behavior tests.
  • Replaces manual JSON field/type checks with require.Contains + assert.Equal for clearer failures.
Show a summary per file
File Description
internal/server/shutdown_test.go Updates shutdown tests with improved testify assertions and enables parallel execution for two top-level test functions.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comments suppressed due to low confidence (1)

internal/server/shutdown_test.go:113

  • Adding t.Parallel() here can make this test run concurrently with other shutdown tests, and /close ultimately calls guard.CloseGlobalCompilationCache() (documented as not safe to call concurrently). This can introduce flakiness/undefined behavior; consider removing t.Parallel() for this test or making the shutdown path avoid/serialize the global cache close in tests.
	t.Parallel()
  • Files reviewed: 1/1 changed files
  • Comments generated: 1

Comment thread internal/server/shutdown_test.go Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@lpcox
lpcox merged commit 2b0d10d into main Apr 30, 2026
15 checks passed
@lpcox
lpcox deleted the test-improver/shutdown-test-improvements-80981fad6765b4ae branch April 30, 2026 13:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants