Skip to content

Conversation

@ADITYATIWARI342005
Copy link

@ADITYATIWARI342005 ADITYATIWARI342005 commented Oct 26, 2025

Which problem is this PR solving?

Description of the changes

  • Added foundational test coverage for the remotesampling extension's Shutdown method
  • This PR focuses specifically on testing shutdown behavior with different component states:
    • TestShutdownWithNilDistributedLock - tests shutdown when distributed lock is nil
    • TestShutdownWithGRPCServer - tests shutdown with active gRPC server
    • TestShutdownWithAllComponents - tests shutdown with multiple active components (strategy provider, HTTP server, gRPC server)
    • TestShutdownWithNilComponents - tests shutdown when all components are nil
  • No code changes required - the existing Shutdown implementation already correctly uses errors.Join() to append errors as recommended in the issue
  • All new tests follow existing patterns and project conventions

Note: This PR provides partial coverage for #7432. Additional tests for Start method error paths and other uncovered lines can be added in follow-up PRs.

How was this change tested?

  • All new tests pass successfully:
  go test ./cmd/jaeger/internal/extension/remotesampling/... -v
  • Test results show 100% pass rate for all 4 new shutdown test cases
  • All existing tests continue to pass
  • Verified with make lint test - no linting errors

Checklist

Add comprehensive test cases for previously uncovered shutdown code paths:
 - TestShutdownWithDistributedLockError: validates shutdown with nil distributed lock
 - TestShutdownWithGRPCServer: tests shutdown with active gRPC server
- TestShutdownWithAllComponents: tests shutdown with multiple active components
 - TestShutdownWithNilComponents: tests shutdown when all components are nil

Signed-off-by: ADITYATIWARI342005 <[email protected]>
@ADITYATIWARI342005 ADITYATIWARI342005 requested a review from a team as a code owner October 26, 2025 10:05
…vior

The test comment incorrectly stated it tests 'when distributed lock fails
to close', but the test actually validates shutdown behavior when the
distributed lock is nil (which should not cause an error).

Updated the comment to accurately describe the test's purpose.

Signed-off-by: ADITYATIWARI342005 <[email protected]>
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.

[chore]: Improve test coverage of remotesampling extension

1 participant