Adding Unit Test Cases For Optimizer REST APIs#8
Open
shekhar316 wants to merge 2 commits intokruize:mvp_demofrom
Open
Adding Unit Test Cases For Optimizer REST APIs#8shekhar316 wants to merge 2 commits intokruize:mvp_demofrom
shekhar316 wants to merge 2 commits intokruize:mvp_demofrom
Conversation
Reviewer's GuideAdds a full REST integration layer around Kruize with datasource/profile/layer/status/jobs/webhook endpoints, centralizes constants and configuration, introduces services for state/profile/datasource management and bulk scheduling, and adds mocked REST-assured tests with JSON fixtures to validate the new API surface. Sequence diagram for scheduled bulk API call and state refreshsequenceDiagram
participant Scheduler as QuarkusScheduler
participant BulkSchedulerService
participant KruizeStateService
participant DatasourceService
participant ProfileService
participant KruizeClient
participant JobsService
%% Startup initialization
Scheduler->>BulkSchedulerService: initialize()
BulkSchedulerService->>KruizeStateService: refreshStateAndInstallProfiles()
KruizeStateService->>DatasourceService: getDatasources()
DatasourceService->>KruizeClient: getDatasources()
KruizeClient-->>DatasourceService: DatasourceListResponse
DatasourceService-->>KruizeStateService: List Datasource
KruizeStateService->>ProfileService: getMetadataProfiles()
ProfileService->>KruizeClient: getMetadataProfiles(verbose=true)
KruizeClient-->>ProfileService: List KruizeProfile
ProfileService-->>KruizeStateService: List KruizeProfile
KruizeStateService->>ProfileService: getMetricProfiles()
ProfileService->>KruizeClient: getMetricProfiles(verbose=true)
KruizeClient-->>ProfileService: List KruizeProfile
ProfileService-->>KruizeStateService: List KruizeProfile
KruizeStateService->>ProfileService: getLayers()
ProfileService->>KruizeClient: getLayers()
KruizeClient-->>ProfileService: List KruizeProfile
ProfileService-->>KruizeStateService: List KruizeProfile
KruizeStateService-->>BulkSchedulerService: state cached
%% Periodic bulk scheduler run
Scheduler->>BulkSchedulerService: scheduledBulkApiCall()
BulkSchedulerService->>KruizeStateService: isCacheEmpty()
KruizeStateService-->>BulkSchedulerService: boolean
alt cache empty
BulkSchedulerService->>KruizeStateService: refreshState()
end
BulkSchedulerService->>KruizeStateService: getDefaultDatasourceName()
KruizeStateService-->>BulkSchedulerService: Optional String
BulkSchedulerService->>KruizeStateService: getDefaultMetadataProfileName()
KruizeStateService-->>BulkSchedulerService: Optional String
BulkSchedulerService->>KruizeStateService: getDefaultMetricProfileName()
KruizeStateService-->>BulkSchedulerService: Optional String
BulkSchedulerService->>BulkSchedulerService: buildBulkPayload()
BulkSchedulerService->>KruizeClient: bulkCreateExperiments(payload)
KruizeClient-->>BulkSchedulerService: String response
BulkSchedulerService->>JobsService: incrementJobsTriggered()
Sequence diagram for webhook handling and jobs statistics updatesequenceDiagram
participant Kruize as KruizeBulkAPI
participant WebhookResource
participant BulkSchedulerService
participant JobsService
Kruize->>WebhookResource: POST /webhook [List WebhookPayload]
WebhookResource->>BulkSchedulerService: handleWebhook(payloads)
loop for each payload
BulkSchedulerService->>BulkSchedulerService: read Summary
alt status COMPLETED and jobId new
BulkSchedulerService->>JobsService: updateExperimentCounters(total, processed, existing)
else status not completed or duplicate
BulkSchedulerService->>BulkSchedulerService: skip update
end
end
BulkSchedulerService-->>WebhookResource: processing done
WebhookResource-->>Kruize: 200 OK
Class diagram for new REST resources, services, and modelsclassDiagram
%% Resources
class DatasourceResource {
+listDatasources() Response
}
class MetadataProfileResource {
+listMetadataProfiles() Response
+installMetadataProfiles() Response
}
class MetricProfileResource {
+listMetricProfiles() Response
+installMetricProfiles() Response
}
class LayerResource {
+listLayers() Response
+installLayers() Response
}
class StatusResource {
+getStatus() Response
}
class JobsResource {
+getJobsOverview() Response
}
class WebhookResource {
+receiveWebhook(List WebhookPayload) Response
}
%% Services
class DatasourceService {
+getDatasources() List~Datasource~
+getDatasourceCount() int
+isKruizeAvailable() boolean
}
class ProfileService {
+getMetadataProfiles() List~KruizeProfile~
+getMetricProfiles() List~KruizeProfile~
+getLayers() List~KruizeProfile~
+installMissingProfiles(profileType String) List~String~
}
class KruizeStateService {
+refreshState() void
+installMissingProfiles() void
+refreshStateAndInstallProfiles() void
+scheduledStateRefresh() void
+getDefaultDatasourceName() Optional~String~
+getDefaultMetadataProfileName() Optional~String~
+getDefaultMetricProfileName() Optional~String~
+getCachedDatasources() List~Datasource~
+getCachedMetadataProfiles() List~KruizeProfile~
+getCachedMetricProfiles() List~KruizeProfile~
+getCachedLayers() List~KruizeProfile~
+isCacheEmpty() boolean
}
class BulkSchedulerService {
+initialize() void
+scheduledBulkApiCall() void
+handleWebhook(List WebhookPayload) void
}
class StatusService {
+getSystemStatus() KruizeStatus
+isKruizeHealthy() boolean
}
class JobsService {
+incrementJobsTriggered() void
+updateExperimentCounters(total int, processed int, existing int) void
+getJobsOverview() JobsOverview
+getTotalJobsTriggered() int
+getTotalExperimentsCreated() int
+getTotalExperimentsProcessed() int
+getTotalExperimentsUnique() int
}
%% Client
class KruizeClient {
<<interface>>
+getDatasources() DatasourceListResponse
+getMetadataProfiles(verbose boolean) List~KruizeProfile~
+getMetricProfiles(verbose boolean) List~KruizeProfile~
+getLayers() List~KruizeProfile~
+createMetadataProfile(profileDefinition Object) String
+createMetricProfile(profileDefinition Object) String
+createLayer(layerDefinition Object) String
+bulkCreateExperiments(bulkRequest Object) String
+getBulkJobStatus(jobId String) String
}
%% Models
class ApiResponse~T~ {
-status String
-message String
-data T
-errors List~String~
+getStatus() String
+getMessage() String
+getData() T
+getErrors() List~String~
+setStatus(status String) void
+setMessage(message String) void
+setData(data T) void
+setErrors(errors List~String~) void
+success(message String, data T) ApiResponse~T~
+success(message String) ApiResponse~T~
+error(message String, errors List~String~) ApiResponse~T~
+error(message String) ApiResponse~T~
}
class DatasourceListResponse {
-datasources List~Datasource~
+getDatasources() List~Datasource~
+setDatasources(datasources List~Datasource~) void
}
class Datasource {
-name String
-provider String
-url String
-serviceName String
-namespace String
+getName() String
+getProvider() String
+getUrl() String
+getServiceName() String
+getNamespace() String
}
class KruizeProfile {
-name String
-profileVersion String
-profileType String
+getName() String
+getProfileVersion() String
+getProfileType() String
+setName(name String) void
+setProfileVersion(profileVersion String) void
+setProfileType(profileType String) void
}
class KruizeStatus {
-datasources DatasourceStatus
-metadataProfiles ProfileStatus
-metricProfiles ProfileStatus
-layers ProfileStatus
-alerts List~String~
+getDatasources() DatasourceStatus
+getMetadataProfiles() ProfileStatus
+getMetricProfiles() ProfileStatus
+getLayers() ProfileStatus
+getAlerts() List~String~
+addAlert(alert String) void
}
class DatasourceStatus {
-count int
-list List~Datasource~
}
class ProfileStatus {
-installed List~ProfileInfo~
}
class ProfileInfo {
-name String
-profileVersion String
}
class JobsOverview {
-jobsTriggered int
-totalExperiments int
-processedExperiments int
-uniqueExperiments int
+getJobsTriggered() int
+getTotalExperiments() int
+getProcessedExperiments() int
+getUniqueExperiments() int
}
class WebhookPayload {
-summary Summary
-webhook WebhookStatus
+getSummary() Summary
+getWebhook() WebhookStatus
}
class Summary {
-jobId String
-status String
-totalExperiments int
-processedExperiments int
-existingExperiments int
}
class WebhookStatus {
-status String
}
%% Exceptions and mappers
class KruizeServiceException {
-statusCode int
+getStatusCode() int
}
class GlobalExceptionMapper {
+toResponse(exception Exception) Response
}
%% Constants container
class OptimizerConstants {
}
class KruizeClientConstants {
}
class OptimizerApiConstants {
}
class GenericOptimizerConstants {
}
class MessageConstants {
}
class ProfileType {
}
class ProfilePathConstants {
}
OptimizerConstants <|-- KruizeClientConstants
OptimizerConstants <|-- OptimizerApiConstants
OptimizerConstants <|-- GenericOptimizerConstants
OptimizerConstants <|-- MessageConstants
OptimizerConstants <|-- ProfileType
OptimizerConstants <|-- ProfilePathConstants
%% Resource to service dependencies
DatasourceResource ..> DatasourceService
MetadataProfileResource ..> ProfileService
MetricProfileResource ..> ProfileService
LayerResource ..> ProfileService
StatusResource ..> StatusService
JobsResource ..> JobsService
WebhookResource ..> BulkSchedulerService
%% Service dependencies
DatasourceService ..> KruizeClient
ProfileService ..> KruizeClient
KruizeStateService ..> DatasourceService
KruizeStateService ..> ProfileService
BulkSchedulerService ..> KruizeClient
BulkSchedulerService ..> KruizeStateService
BulkSchedulerService ..> JobsService
StatusService ..> DatasourceService
StatusService ..> ProfileService
StatusService ..> KruizeStateService
JobsService ..> JobsOverview
%% Model usage
DatasourceService ..> DatasourceListResponse
DatasourceListResponse o--> Datasource
KruizeStatus o--> DatasourceStatus
KruizeStatus o--> ProfileStatus
ProfileStatus o--> ProfileInfo
WebhookPayload o--> Summary
WebhookPayload o--> WebhookStatus
%% Exception mapping
GlobalExceptionMapper ..> KruizeServiceException
GlobalExceptionMapper ..> ApiResponse~Object~
%% Generic ApiResponse usage
DatasourceResource ..> ApiResponse~List~
MetadataProfileResource ..> ApiResponse~List~
MetricProfileResource ..> ApiResponse~List~
LayerResource ..> ApiResponse~List~
StatusResource ..> ApiResponse~KruizeStatus~
JobsResource ..> ApiResponse~JobsOverview~
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The
KruizeClientis registered with@RegisterRestClient(configKey = "kruize-api")butapplication.ymlconfiguresquarkus.rest-client."com.kruize.optimizer.client.KruizeClient", which means the main app won’t pick up the intended URL; align the config key and property name so the client is actually using the configured endpoint. - In
BulkSchedulerService.parseTargetLabels, you’re manually parsing JSON using string operations even though anObjectMapperis available; consider using Jackson (readTreeor binding to a small DTO) to make this more robust to whitespace/escaping and easier to extend (e.g., support array forms). - Most resource methods wrap their bodies in broad try/catch and return error
ApiResponses, while aGlobalExceptionMapperis also introduced; to reduce duplication and keep error handling consistent, consider letting services throwKruizeServiceExceptionand relying on the mapper instead of catching genericExceptionin each resource.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `KruizeClient` is registered with `@RegisterRestClient(configKey = "kruize-api")` but `application.yml` configures `quarkus.rest-client."com.kruize.optimizer.client.KruizeClient"`, which means the main app won’t pick up the intended URL; align the config key and property name so the client is actually using the configured endpoint.
- In `BulkSchedulerService.parseTargetLabels`, you’re manually parsing JSON using string operations even though an `ObjectMapper` is available; consider using Jackson (`readTree` or binding to a small DTO) to make this more robust to whitespace/escaping and easier to extend (e.g., support array forms).
- Most resource methods wrap their bodies in broad try/catch and return error `ApiResponse`s, while a `GlobalExceptionMapper` is also introduced; to reduce duplication and keep error handling consistent, consider letting services throw `KruizeServiceException` and relying on the mapper instead of catching generic `Exception` in each resource.
## Individual Comments
### Comment 1
<location path="src/main/java/com/kruize/optimizer/exception/GlobalExceptionMapper.java" line_range="28-36" />
<code_context>
+ * Global exception mapper for handling exceptions across the application
+ */
+@Provider
+public class GlobalExceptionMapper implements ExceptionMapper<Exception> {
+
+ private static final Logger LOG = Logger.getLogger(GlobalExceptionMapper.class);
+
+ @Override
+ public Response toResponse(Exception exception) {
+ LOG.error("Exception occurred", exception);
+
+ if (exception instanceof KruizeServiceException) {
+ KruizeServiceException kse = (KruizeServiceException) exception;
+ return Response.status(kse.getStatusCode())
</code_context>
<issue_to_address>
**issue (bug_risk):** Catching all Exception types here may inadvertently override HTTP status codes from WebApplicationException and similar.
Since this mapper is registered for the base `Exception` type, it will also intercept `WebApplicationException` (and subclasses like `NotFoundException`) and map them to 500 unless they are `KruizeServiceException`. This discards explicitly set HTTP status codes. Consider either narrowing the mapper’s type (e.g., to `RuntimeException`) or adding logic that detects `WebApplicationException` and preserves its `getResponse().getStatus()` (and possibly its entity).
</issue_to_address>
### Comment 2
<location path="src/main/java/com/kruize/optimizer/service/BulkSchedulerService.java" line_range="166-175" />
<code_context>
+ private Map<String, String> parseTargetLabels() {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Manual string parsing of JSON labels is fragile; consider leveraging ObjectMapper or a typed config.
Splitting on commas/colons and stripping quotes will fail for values that contain those characters, extra whitespace, or any nested/structured content, and can easily drift from the actual config format. Since `ObjectMapper` is already available, parse `targetLabelsJson` into a `Map<String, String>` (or a small DTO) instead to eliminate these edge cases and reduce maintenance.
Suggested implementation:
```java
import com.fasterxml.jackson.core.type.TypeReference;
import com.fasterxml.jackson.databind.ObjectMapper;
```
```java
import java.util.Collections;
import java.util.HashMap;
```
```java
/**
* Parse target labels from JSON configuration
*
* @return Map of label key-value pairs
*/
private Map<String, String> parseTargetLabels() {
if (targetLabelsJson == null || targetLabelsJson.isBlank()) {
return Collections.emptyMap();
}
try {
// Parse JSON directly into a Map<String, String> using ObjectMapper
return objectMapper.readValue(
targetLabelsJson,
new TypeReference<Map<String, String>>() {}
);
```
To fully apply the suggestion and remove the fragile manual parsing:
1. Delete the remainder of the old `parseTargetLabels` method body after the `try {` shown in the SEARCH block, including:
- The `Map<String, String> labels = new HashMap<>();` line
- All the manual string operations (`substring`, `split`, quote stripping, etc.)
- Any `catch` block that was handling parsing failures for the manual logic.
2. Replace that deleted code with a proper `catch` block matching the new implementation, for example:
```java
} catch (Exception e) {
LOG.errorf(e, "Failed to parse targetLabelsJson, returning empty labels. Raw value: %s", targetLabelsJson);
return Collections.emptyMap();
}
```
3. Ensure that:
- `objectMapper` is available as a field in this class (e.g., injected via `@Inject ObjectMapper objectMapper;` or constructor).
- There is no remaining manual string-based parsing of `targetLabelsJson` anywhere else in the method.
</issue_to_address>
### Comment 3
<location path="src/main/java/com/kruize/optimizer/service/StatusService.java" line_range="55-64" />
<code_context>
+ // Refresh the global state cache
</code_context>
<issue_to_address>
**suggestion (performance):** StatusService refreshes the cache and then bypasses it by calling ProfileService directly, causing redundant remote calls.
In `getSystemStatus()`, `kruizeStateService.refreshState()` already calls the datasource and profile services, but you then call `profileService.getMetadataProfiles()`, `getMetricProfiles()`, and `getLayers()` again. This doubles the remote calls per request. Prefer using the cached values from `KruizeStateService` (e.g., `getCachedMetadataProfiles()`, etc.) after the refresh to avoid the extra calls and keep a single source of truth.
Suggested implementation:
```java
// Get metadata profiles from cache (already refreshed above)
List<MetadataProfile> metadataProfiles = kruizeStateService.getCachedMetadataProfiles();
```
```java
// Get metric profiles from cache (already refreshed above)
List<MetricProfile> metricProfiles = kruizeStateService.getCachedMetricProfiles();
```
```java
// Get layers from cache (already refreshed above)
List<Layer> layers = kruizeStateService.getCachedLayers();
```
If `getSystemStatus()` uses `profileService` anywhere else (e.g., for other profile-related data), those calls should also be replaced with the corresponding `kruizeStateService.getCached*()` methods so that:
1) `kruizeStateService.refreshState()` is the single source of truth for refreshing remote state, and
2) all reads use the in-memory cached values instead of issuing extra remote calls.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@shekhar316 Can you update the description of the PR to provide more insights on what this PR is for? |
Contributor
Author
|
@rbadagandi1 sure, I have updated summary to be more descriptive. This PR aims to add unit test cases using mockito for the optimizer microservice. Detailed description of tests and methods are already added to PR description. |
Contributor
Author
|
Contributor
Author
|
@pinkygupta-hub can you please review this? |
| @BeforeEach | ||
| void setUp() throws IOException { | ||
| Mockito.reset(bulkSchedulerService, kruizeStateService, kruizeClient); | ||
|
|
There was a problem hiding this comment.
I feel like we’re mocking almost everything here (including client/service interactions), which makes these tests more about verifying mocks than actually testing how the resource behaves.
A couple of concerns:
The actual flow isn’t really being exercised
We’re mostly validating mocked responses instead of real logic
This could lead to tests passing even if the real integration is broken
Maybe we can keep external dependencies mocked, but let the resource- service flow run with real logic where possible. That would give us more confidence in the actual behavior.
@shekhar316
Contributor
Author
There was a problem hiding this comment.
Thanks for the feedback! I understand your concern about the mocking approach.
Why We Mock KruizeClient:
- KruizeClient is an external service dependency that requires a running Kruize instance
- Mocking it allows tests to run without any external setup or infrastructure
- We use actual API responses (stored as JSON files) to ensure our mocks reflect real behavior
- This keeps tests fast, reliable, and independent of external services
Why We Mock BulkSchedulerService:
- BulkSchedulerService performs initialization and startup tasks that aren't relevant to testing the MetricProfile API endpoints
- Mocking it prevents unnecessary setup overhead during test execution
What We're Actually Testing:
- Despite the mocks, we're still testing the real business logic:
- MetricProfileResource - Real implementation (request handling, response formatting)
- ProfileService - Real implementation (error handling, data transformation, business logic)
- KruizeClient - Mocked with actual API responses (external boundary)
E2E Testing:
You're absolutely right that we'll have dedicated E2E tests and integration tests also later that:
- Run against a real Kruize instance
- Validate the complete integration end-to-end
- Catch any issues with actual API contracts or behavior
Signed-off-by: Shekhar Saxena <shekhar.saxena@ibm.com>
Signed-off-by: Shekhar Saxena <shekhar.saxena@ibm.com>
Contributor
Author
|
|
@chandrams Can you please review this |
| @@ -0,0 +1,203 @@ | |||
| [ | |||
There was a problem hiding this comment.
Instead of having the static mock responses, can we have it programatically determined based on the configs we have in code ? Whenever there are changes related to configs, we need to update all these. It becomes a maintenance overhead to the team
Contributor
Author
There was a problem hiding this comment.
Agree with your point, we should definitely test with different scenarios rather than relying on fixed static files. The initial design for the unit tests was intended to cover the complete flow (KruizeClient → ProfileService → LayerResource), not just ProfileService in isolation and just to test the basic validation if that service works.
Since Kruize isn’t available during unit tests, converting the layers JSON to match Kruize layers API responses for diff scenarios could add some overhead, especially considering these responses don’t change frequently.
And, we do have integration tests planned with a live Kruize instance, which will catch any config/response mismatches across multiple scenarios.
If you think it would be helpful, I can add the additional scenario-based and programmatic generation of JSONs for testing to the unit tests, please let me know your thoughts.
Contributor
Author
|
Hi @chandrams, can you please review it whenever you have some bandwidth? Thanks. |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.
Summary
This PR adds the unit test cases of REST API endpoints in Optimizer application. These unit test cases aim to test the basic functionality of REST API endpoints present in the applications which are related to checking the installed profiles, layers and datasources in kruize app.
Changes
Added unit tests for the following resource endpoints:
DatasourceResourceTest- Tests for datasource API endpointsLayerResourceTest- Tests for layer configuration endpointsMetadataProfileResourceTest- Tests for metadata profile endpointsMetricProfileResourceTest- Tests for metric profile endpointsWebhookResourceTest- Tests for webhook endpointsCreated
MockResponseLoaderutility class to load mock JSON responses from test resourcesAdded mock JSON response files for various API scenarios:
Scope & Limitations
Future Work
More detailed test cases will be added in subsequent PRs, including:
Test Coverage: Basic API endpoint tests with mock responses only
Summary by Sourcery
Add REST client, services, resources, and models to integrate with Kruize APIs for datasources, profiles, layers, jobs, status, and webhooks, along with configuration updates and supporting utilities.
New Features:
Enhancements:
Build:
Documentation:
Tests: