[Health]: Test Case for Rest and GraphQL Requests to DAB during health check #2667
Conversation
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
added 3 commits
April 18, 2025 12:55
… dev/sezalchug/healthCheckTestCase
sezal98
requested review from
Aniruddh25,
aaronburtle,
RubenCerna2079,
ravishetye,
neeraj-sharma2592 and
sourabh1007
as code owners
There was a problem hiding this comment.
Pull Request Overview
This PR adds unit tests for REST and GraphQL health check endpoints while refactoring the HTTP client configuration and health check utilities. Key changes include:
- Configuring a named HttpClient in Startup to set up base addresses and default headers.
- Refactoring HttpUtilities methods to use IHttpClientFactory with asynchronous REST calls.
- Updating test configurations and helper methods to support asynchronous health check processing.
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/Service/Startup.cs | Adds a named HttpClient for context-aware health check requests. |
| src/Service/HealthCheck/Utilities.cs | Updates REST query string formatting for health check endpoints. |
| src/Service/HealthCheck/HttpUtilities.cs | Refactors REST calls to async using IHttpClientFactory and updates GraphQL query handling. |
| src/Service/HealthCheck/HealthCheckHelper.cs | Converts helper methods to async to support non-blocking health check responses. |
| src/Service/HealthCheck/ComprehensiveHealthReportResponseWriter.cs | Updates response writing to be asynchronous. |
| src/Service.Tests/Configuration/ConfigurationTests.cs | Removes unused options in custom configuration file creation and hardcodes GraphQL enablement. |
| src/Core/Services/MetadataProviders/ISqlMetadataProvider.cs | Adds a helper method for accessing database objects by key. |
| src/Config/DatabasePrimitives/DatabaseObject.cs | Allows overriding of the SourceDefinition property for testability. |
Comments suppressed due to low confidence (2)
src/Service/HealthCheck/HttpUtilities.cs:122
- For consistency with the async ExecuteRestQuery, consider converting ExecuteGraphQLQuery to an async method and await the HttpClient call instead of using .Result.
public string? ExecuteGraphQLQuery(string graphqlUriSuffix, string entityName, Entity entity, string incomingRoleHeader, string incomingRoleToken)
src/Service.Tests/Configuration/ConfigurationTests.cs:4457
- Since the function previously accepted multiple flags for GraphQL and health check enablement, ensure that hardcoding GraphQL as enabled aligns with the intended test scenarios and document the change accordingly.
private static void CreateCustomConfigFile(Dictionary<string, Entity> entityMap, bool enableGlobalRest = true)
|
/azp run |
|
Azure Pipelines successfully started running 6 pipeline(s). |
Aniruddh25
reviewed
Apr 29, 2025
added 4 commits
April 29, 2025 13:02
… dev/sezalchug/healthCheckTestCase
|
/azp run |
|
Azure Pipelines successfully started running 6 pipeline(s). |
Aniruddh25
reviewed
May 1, 2025
| { | ||
| Assert.Fail(); | ||
| } | ||
| HealthEndpointTests.ValidateBasicDetailsHealthCheckResponse(responseProperties); |
There was a problem hiding this comment.
nit: missing tab to align with previous code
Aniruddh25
reviewed
May 1, 2025
| @@ -4667,22 +4454,20 @@ public async Task TestNoDepthLimitOnGrahQLInNonHostedMode(int? depthLimit) | |||
| /// </summary> | |||
| /// <param name="entityMap">Collection of entityName -> Entity object.</param> | |||
| /// <param name="enableGlobalRest">flag to enable or disabled REST globally.</param> | |||
| private static void CreateCustomConfigFile(Dictionary<string, Entity> entityMap, bool enableGlobalRest = true, bool enableGlobalGraphql = true, bool enableGlobalHealth = true, bool enableDatasourceHealth = true, HostMode hostMode = HostMode.Production) | |||
| private static void CreateCustomConfigFile(Dictionary<string, Entity> entityMap, bool enableGlobalRest = true) | |||
There was a problem hiding this comment.
Is enableGlobalGraphQL not used by any of the existing tests?
Aniruddh25
reviewed
May 1, 2025
| @@ -270,6 +271,27 @@ public void ConfigureServices(IServiceCollection services) | |||
| //Enable accessing HttpContext in RestService to get ClaimsPrincipal. | |||
| services.AddHttpContextAccessor(); | |||
|
|
|||
| services.AddHttpClient("ContextConfiguredHealthCheckClient") | |||
| .ConfigureHttpClient((serviceProvider, client) => | |||
| { | |||
There was a problem hiding this comment.
does adding HttpClient to the service provider help with testing?
Aniruddh25
reviewed
May 1, 2025
| // Act | ||
| string errorMessageFromGraphQL = httpUtilities.ExecuteGraphQLQuery( | ||
| graphqlUriSuffix: "/graphql", | ||
| entityName: runtimeConfig.Entities.First().Key, |
There was a problem hiding this comment.
Would this test have caught the bug reported in this #2645 ?
Aniruddh25
reviewed
May 1, 2025
| incomingRoleToken: string.Empty); | ||
|
|
||
| // Assert | ||
| Assert.IsNull(errorMessageFromGraphQL); |
There was a problem hiding this comment.
We should have a test for the failure case as well. Mock a failed response, and verify the httputilities returns the error
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Why make this change?
Resolves #2662
What is this change?
How was this tested?
Sample Request(s)
Requests are running as expected. Test cases added for Rest and Graphql