Skip to content

Conversation

mohamedfawas
Copy link
Contributor

Summary

This PR adds Priority 1 (Core Integration) tests for the token-exchange middleware integration as described in issue #2149.

Changes

  • Added unit tests in pkg/runner/config_builder_test.go
  • Covered:
    • Middleware not added when tokenExchangeConfig is nil
    • Middleware added and correctly ordered when config is provided (auth → token-exchange → MCP parser)
    • Middleware chain integrity
  • All tests pass locally

Scope

This PR focuses only on Priority 1 as defined in the issue description.
I plan to address Priority 2 and 3 in separate follow-up PRs.

Related Issue

Closes #2149

@eleftherias eleftherias requested a review from jhrozek October 16, 2025 12:55
Copy link

codecov bot commented Oct 16, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 53.37%. Comparing base (69a7f69) to head (86f9fe1).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2227      +/-   ##
==========================================
+ Coverage   53.32%   53.37%   +0.05%     
==========================================
  Files         231      231              
  Lines       29536    29536              
==========================================
+ Hits        15749    15766      +17     
+ Misses      12654    12636      -18     
- Partials     1133     1134       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

@jhrozek jhrozek left a comment

Choose a reason for hiding this comment

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

Thanks for adding these tests! They correctly verify the middleware ordering and types.

One suggestion: the tests currently only check the Type field of each middleware config. Consider also verifying that the Parameters field is properly populated and contains the expected configuration values. This would catch issues where the middleware has the correct type but is missing or has incorrect configuration data.

For the second test case where you provide a token-exchange config, you could:

  1. Use actual config values (TokenURL, ClientID, etc.) instead of an empty struct
  2. Assert that the Parameters field is not nil
  3. Unmarshal the Parameters and verify the config was passed through correctly

This would give more confidence that the middleware is fully configured, not just present in the chain.

@mohamedfawas mohamedfawas force-pushed the test/token-exchange-integration branch from ea92a77 to 835667e Compare October 19, 2025 01:59
@JAORMX JAORMX requested a review from Copilot October 19, 2025 03:32
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Adds Priority 1 integration tests validating token-exchange middleware presence, ordering, and parameter propagation in the core middleware chain.

  • Adds tests for absence when config is nil
  • Adds tests for presence, ordering (auth → token-exchange → MCP parser), and parameter serialization

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Add Priority 1 (Core Integration) unit tests for token-exchange middleware in pkg/runner/config_builder.go.

These tests:
- Verify middleware is not added when tokenExchangeConfig is nil
- Verify middleware is added and correctly ordered (auth → token-exchange → MCP parser)
- Validate middleware chain integrity

Fixes: stacklok#2149

Signed-off-by: Mohamed Fawas <[email protected]>
This commit adds Priority 1 (Core Integration) tests for the token-exchange middleware in pkg/runner/config_builder_test.go.

Changes include:
- Verified middleware is not added when `tokenExchangeConfig` is nil.
- Verified middleware order (auth → token-exchange → mcp parser) when config is provided.
- Confirmed that middleware parameters are populated and match the provided `tokenexchange.Config`.

This improves coverage and ensures correct integration behavior between authentication and token-exchange components.

Signed-off-by: Mohamed Fawas <[email protected]>
@mohamedfawas mohamedfawas force-pushed the test/token-exchange-integration branch from dad9fbc to 86f9fe1 Compare October 19, 2025 03:44
@mohamedfawas
Copy link
Contributor Author

Hi @jhrozek 👋

Thank you for the review and detailed suggestions.

I’ve updated the tests based on your feedback:

  • Added realistic tokenexchange.Config values (TokenURL, ClientID, ClientSecret, etc.)
  • Asserted that the Parameters field is populated (non-nil and non-empty)
  • Unmarshaled Parameters and verified that all config values are correctly propagated into tokenexchange.MiddlewareParams

All tests pass locally.
go test ./pkg/runner -run TestAddCoreMiddlewares_TokenExchangeIntegration -v

This update ensures the middleware is not only present in the chain but also fully configured as expected.

This PR now fully covers Priority 1 (Core Integration) from issue #2149.
I will handle Priority 2 and Priority 3 in separate follow-up PRs to keep the scope focused.

Ready for re-review — thanks again for your guidance!

@jhrozek jhrozek merged commit 783d0bc into stacklok:main Oct 20, 2025
42 of 43 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add test coverage for token exchange middleware integration

2 participants