Skip to content

Fix missing MCP test #2925

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Fix missing MCP test #2925

wants to merge 9 commits into from

Conversation

@RubenCerna2079
Copy link
Contributor

Why make this change?

  • Refine Service.Tests changes for MCP #2874
    There was a test that needed to be fixed in order to comply with the creation of the new MCP endpoint. The test already existed and covered the scenarios for REST and GraphQL endpoints. The reason this test needed to be fixed separately is that it was failing when it was changed to comply with the MCP endpoint.

What is this change?

This change fixes the test that was failing, the reason it was failing is that the MCP server was not able to start in time before the test tried to access the MCP endpoint. In order to fix it we added a delay so the server is available before the test tries to access the endpoint.

How was this tested?

  • Integration Tests
  • Unit Tests

@RubenCerna2079 RubenCerna2079 added this to the Oct 2025 milestone Oct 27, 2025
@RubenCerna2079 RubenCerna2079 self-assigned this Oct 27, 2025
Copilot AI review requested due to automatic review settings October 27, 2025 19:44
@RubenCerna2079 RubenCerna2079 changed the title Dev/rubencerna/mcp fix tests Fix missing MCP test Oct 27, 2025
Copilot AI reviewed Oct 27, 2025
View reviewed changes
Copy link
Contributor

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

This PR fixes a failing test for MCP (Model Context Protocol) endpoint support. The test was extended to cover MCP endpoints alongside existing REST and GraphQL endpoint tests, but failed due to the MCP server not being ready in time.

Key changes:

  • Extended test parameters to include MCP endpoint testing alongside REST and GraphQL
  • Added a 2-second delay before MCP endpoint testing to allow server initialization
  • Created a helper method GetMcpResponsePostConfigHydration with retry logic similar to existing GraphQL helper

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

src/Service.Tests/Configuration/ConfigurationTests.cs
[DataRow(true, true, true, HttpStatusCode.OK, HttpStatusCode.OK, HttpStatusCode.OK, CONFIGURATION_ENDPOINT, DisplayName = "V1 - Rest, GraphQL, and MCP enabled globally")]
[DataRow(true, true, false, HttpStatusCode.OK, HttpStatusCode.OK, HttpStatusCode.NotFound, CONFIGURATION_ENDPOINT, DisplayName = "V1 - Rest and GraphQL enabled, MCP disabled globally")]
[DataRow(true, false, true, HttpStatusCode.OK, HttpStatusCode.NotFound, HttpStatusCode.OK, CONFIGURATION_ENDPOINT, DisplayName = "V1 - Rest enabled, GraphQL disabled, and MCP enabled globally")]
[DataRow(true, false, false, HttpStatusCode.OK, HttpStatusCode.NotFound, HttpStatusCode.NotFound, CONFIGURATION_ENDPOINT, DisplayName = "V1 - Rest enabled, GraphQL and MCP enabled globally")]
Copy link

Copilot AI Oct 27, 2025

Choose a reason for hiding this comment

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

The display name incorrectly states 'GraphQL and MCP enabled globally' when the test parameters show both are disabled (false, false). Should be 'GraphQL and MCP disabled globally'.

Suggested change
[DataRow(true, false, false, HttpStatusCode.OK, HttpStatusCode.NotFound, HttpStatusCode.NotFound, CONFIGURATION_ENDPOINT, DisplayName = "V1 - Rest enabled, GraphQL and MCP enabled globally")]
[DataRow(true, false, false, HttpStatusCode.OK, HttpStatusCode.NotFound, HttpStatusCode.NotFound, CONFIGURATION_ENDPOINT, DisplayName = "V1 - Rest enabled, GraphQL and MCP disabled globally")]

Copilot uses AI. Check for mistakes.
src/Service.Tests/Configuration/ConfigurationTests.cs
[DataRow(true, true, true, HttpStatusCode.OK, HttpStatusCode.OK, HttpStatusCode.OK, CONFIGURATION_ENDPOINT_V2, DisplayName = "V2 - Rest, GraphQL, and MCP enabled globally")]
[DataRow(true, true, false, HttpStatusCode.OK, HttpStatusCode.OK, HttpStatusCode.NotFound, CONFIGURATION_ENDPOINT_V2, DisplayName = "V2 - Rest and GraphQL enabled, MCP disabled globally")]
[DataRow(true, false, true, HttpStatusCode.OK, HttpStatusCode.NotFound, HttpStatusCode.OK, CONFIGURATION_ENDPOINT_V2, DisplayName = "V2 - Rest enabled, GraphQL disabled, and MCP enabled globally")]
[DataRow(true, false, false, HttpStatusCode.OK, HttpStatusCode.NotFound, HttpStatusCode.NotFound, CONFIGURATION_ENDPOINT_V2, DisplayName = "V2 - Rest enabled, GraphQL and MCP enabled globally")]
Copy link

Copilot AI Oct 27, 2025

Choose a reason for hiding this comment

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

The display name incorrectly states 'GraphQL and MCP enabled globally' when the test parameters show both are disabled (false, false). Should be 'GraphQL and MCP disabled globally'.

Suggested change
[DataRow(true, false, false, HttpStatusCode.OK, HttpStatusCode.NotFound, HttpStatusCode.NotFound, CONFIGURATION_ENDPOINT_V2, DisplayName = "V2 - Rest enabled, GraphQL and MCP enabled globally")]
[DataRow(true, false, false, HttpStatusCode.OK, HttpStatusCode.NotFound, HttpStatusCode.NotFound, CONFIGURATION_ENDPOINT_V2, DisplayName = "V2 - Rest enabled, GraphQL and MCP disabled globally")]

Copilot uses AI. Check for mistakes.
src/Service.Tests/Configuration/ConfigurationTests.cs Outdated
Comment on lines 2599 to 2610
await Task.Delay(2000);
object mcpPayload = new
{
jsonrpc = "2.0",
id = 1,
method = "tools/call"
};
HttpRequestMessage mcpRequest = new(HttpMethod.Post, "/mcp")
{
Content = JsonContent.Create(mcpPayload)
};
HttpResponseMessage mcpResponse = await client.SendAsync(mcpRequest);
Copy link

Copilot AI Oct 27, 2025

Choose a reason for hiding this comment

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

The hardcoded 2000ms delay introduces brittleness and slows down test execution. Consider using the existing retry pattern (like GetMcpResponsePostConfigHydration) or defining this delay as a named constant with documentation explaining why it's necessary.

Suggested change
await Task.Delay(2000);
object mcpPayload = new
{
jsonrpc = "2.0",
id = 1,
method = "tools/call"
};
HttpRequestMessage mcpRequest = new(HttpMethod.Post, "/mcp")
{
Content = JsonContent.Create(mcpPayload)
};
HttpResponseMessage mcpResponse = await client.SendAsync(mcpRequest);
object mcpPayload = new
{
jsonrpc = "2.0",
id = 1,
method = "tools/call"
};
HttpResponseMessage mcpResponse = await GetMcpResponsePostConfigHydration(client, mcpPayload);

Copilot uses AI. Check for mistakes.
src/Service.Tests/Configuration/ConfigurationTests.cs Outdated
Comment on lines 2599 to 2611
await Task.Delay(2000);
object mcpPayload = new
{
jsonrpc = "2.0",
id = 1,
method = "tools/call"
};
HttpRequestMessage mcpRequest = new(HttpMethod.Post, "/mcp")
{
Content = JsonContent.Create(mcpPayload)
};
HttpResponseMessage mcpResponse = await client.SendAsync(mcpRequest);
Assert.AreEqual(expectedStatusCodeForMcp, mcpResponse.StatusCode);
Copy link

Copilot AI Oct 27, 2025

Choose a reason for hiding this comment

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

This MCP request logic duplicates the payload and request construction found in GetMcpResponsePostConfigHydration. Consider extracting this into a shared helper method to reduce duplication and ensure consistency.

Copilot uses AI. Check for mistakes.
@RubenCerna2079
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 6 pipeline(s).

@RubenCerna2079
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 6 pipeline(s).

@RubenCerna2079
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 6 pipeline(s).

@RubenCerna2079
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 6 pipeline(s).

@RubenCerna2079
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 6 pipeline(s).

@RubenCerna2079
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 6 pipeline(s).

@RubenCerna2079
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 6 pipeline(s).

Aniruddh25
Aniruddh25 reviewed Oct 30, 2025
View reviewed changes
src/Service.Tests/Configuration/ConfigurationTests.cs
Content = JsonContent.Create(mcpPayload)
};
mcpRequest.Headers.Add("Accept", "*/*");
HttpResponseMessage mcpResponse = await client.SendAsync(mcpRequest);
Copy link
Collaborator

Choose a reason for hiding this comment

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

why cant we use the same GetMcpResponsePostConfigHydration method here to account for the retry logic?

Aniruddh25
Aniruddh25 requested changes Oct 30, 2025
View reviewed changes
Copy link
Collaborator

@Aniruddh25 Aniruddh25 left a comment

Choose a reason for hiding this comment

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

Looking for retry logic

@github-project-automation github-project-automation bot moved this from Todo to Review In Progress in Data API builder Oct 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@Aniruddh25 Aniruddh25 Aniruddh25 requested changes

@aaronburtle aaronburtle Awaiting requested review from aaronburtle aaronburtle is a code owner

@anushakolan anushakolan Awaiting requested review from anushakolan anushakolan is a code owner

@souvikghosh04 souvikghosh04 Awaiting requested review from souvikghosh04 souvikghosh04 is a code owner

@neeraj-sharma2592 neeraj-sharma2592 Awaiting requested review from neeraj-sharma2592 neeraj-sharma2592 is a code owner

@sourabh1007 sourabh1007 Awaiting requested review from sourabh1007 sourabh1007 is a code owner

@vadeveka vadeveka Awaiting requested review from vadeveka vadeveka is a code owner

@Alekhya-Polavarapu Alekhya-Polavarapu Awaiting requested review from Alekhya-Polavarapu Alekhya-Polavarapu is a code owner

@rusamant rusamant Awaiting requested review from rusamant rusamant is a code owner

Requested changes must be addressed to merge this pull request.

Assignees

@RubenCerna2079 RubenCerna2079

Labels

None yet

Projects

Data API builder
Status: Review In Progress

Milestone

Oct 2025

Development

Successfully merging this pull request may close these issues.

3 participants

@RubenCerna2079 @Aniruddh25