test(MonitoredDeployBaseTask): add tests to verify log message for different types of RetrofitError #4608

Pranav-b-7 · 2023-11-22T10:40:55Z

This PR introduces test cases designed to assess the current behaviour of the getRetrofitLogMessage() method when faced with diverse RetrofitErrors, including HTTP Error, Network Error, Unexpected Error, and Conversion Error. These tests provide a comprehensive understanding of the existing functionality before upcoming enhancements.

The primary objective of these test cases is to serve as a reference point for the behaviour of getRetrofitLogMessage() prior to planned modifications. The intention is to eliminate the usage of RetrofitError, and the added tests will help validate and compare the code's behaviour after the proposed changes.

Details:

Added specific test cases to simulate HTTP Error, Network Error, Unexpected Error, and Conversion Error scenarios.
These tests are crucial in establishing a baseline for the current behavior of getRetrofitLogMessage().
The subsequent changes will focus on refactoring the method to enhance its reliability and maintainability by eliminating reliance on RetrofitError.

...va/com/netflix/spinnaker/orca/clouddriver/tasks/monitoreddeploy/MonitoredDeployBaseTask.java

...lix/spinnaker/orca/clouddriver/tasks/monitoreddeploy/EvaluateDeploymentHealthTaskSpec.groovy

...om/netflix/spinnaker/orca/clouddriver/tasks/monitoreddeploy/MonitoredDeployBaseTaskTest.java

…fferent types of RetrofitError This commit introduces a set of test cases aimed at showcasing the current behavior of the getRetrofitLogMessage() method when different types of RetrofitErrors, such as HTTP Error, Network Error, Unexpected Error, and Conversion Error, are thrown. The purpose of these test cases is to establish a baseline understanding of the method's behavior in handling various RetrofitError scenarios before the upcoming modifications. These tests will serve as a reference point to compare and validate the behavior of the code after enhancements are made, specifically focusing on eliminating the usage of RetrofitError. These test cases provide valuable insight into the existing behavior of the getRetrofitLogMessage() method and will aid in assessing the effectiveness of subsequent modifications.

…for DeploymentMonitorService Update the error handler for the retrofit client, DeploymentMonitorService, to use SpinnakerRetrofitErrorHandler. Additionally, modify catch blocks with RetrofitError to use Spinnaker*Exception. Purpose : This commit is foundational work for upgrading the Retrofit version to retrofit2.x Behavioural change : The modification includes updating catch blocks, leading to a change in the format of logger messages. This aligns with the upcoming modifications discussed in the PR: spinnaker#4608

…for DeploymentMonitorService Update the error handler for the retrofit client, DeploymentMonitorService, to use SpinnakerRetrofitErrorHandler. Additionally, modify catch blocks with RetrofitError to use Spinnaker*Exception. Purpose : This commit is foundational work for upgrading the Retrofit version to retrofit2.x Behavioural change : The modification includes updating catch blocks, leading to a change in the format of logger messages. This aligns with the upcoming modifications discussed in the PR: spinnaker#4608 Additionally, a new specific exception handler is introduced to manage JsonProcessing during the serialization of the HTTP error body. The impact of these changes should be compared with another related PR: spinnaker#4617

…for DeploymentMonitorService Update the error handler for the retrofit client, DeploymentMonitorService, to use SpinnakerRetrofitErrorHandler. Additionally, modify catch blocks with RetrofitError to use Spinnaker*Exception. Purpose : This commit is foundational work for upgrading the Retrofit version to retrofit2.x Behavioural change : The modification includes updating catch blocks, leading to a change in the format of logger messages. This aligns with the upcoming modifications discussed in the PR: spinnaker#4608 Additionally, when the error response body is null, an appropriate error message will be printed in the logger.

…e for different types of RetrofitError (spinnaker#4608)" This reverts commit adc81ac. With the addition of EvaluateDeploymentHealthTaskTest, we have equivalent coverage that: - is in java instead of groovy - doesn't increase the visibility of getRetrofitLogMessage - more easily shows behavior changes when adopting SpinnakerRetrofitErrorHandler - is less likely to change when moving from retrofit to retrofit2

…eploymentMonitorService (#4628) * refactor(clouddriver/test): configure logback to log messages to make it possible to add tests that assert on the contents of log messages. orca-test has a logback-test.xml file that disables logging, so add one for orca-clouddriver that takes precedence so log messages actually get generated. Now that logging is enabled, clock.millis() is called an additional time during a test in ServerGroupCacheForceRefreshTaskSpec. * test(clouddriver): demonstrate behavior of EvaluateDeploymentHealthTask to see what changes when moving to SpinnakerRetrofitErrorHandler. Specifically, these tests verify the contents of a log message that's tightly coupled to RetrofitError. * Revert "test(MonitoredDeployBaseTask): Add tests to verify log message for different types of RetrofitError (#4608)" This reverts commit adc81ac. With the addition of EvaluateDeploymentHealthTaskTest, we have equivalent coverage that: - is in java instead of groovy - doesn't increase the visibility of getRetrofitLogMessage - more easily shows behavior changes when adopting SpinnakerRetrofitErrorHandler - is less likely to change when moving from retrofit to retrofit2 * refactor(deploymentmonitor): use SpinnakerRetrofitErrorHandler with DeploymentMonitorService Note, there's a behavior change here when http response bodies aren't json objects. Previously, the log message would, barring an exception processing the response, include the http response body in the log message. With SpinnakerHttpException, response bodies that aren't json objects aren't available, so the body appears to be empty. For example, before: 2024-01-04 12:31:59.640 WARN --- [ Test worker] n.s.o.c.t.m.EvaluateDeploymentHealthTask : [] HTTP Error encountered while talking to monitorName(monitorId)->http://localhost:49179/deployment/evaluateHealth, status: 400 (Bad Request) response body: non-json response} retrofit.RetrofitError: 400 Bad Request at retrofit.RetrofitError.httpError(RetrofitError.java:40) after: 2024-01-04 14:26:13.270 WARN --- [ Test worker] n.s.o.c.t.m.EvaluateDeploymentHealthTask : [] HTTP Error encountered while talking to monitorName(monitorId)->http://localhost:58192/deployment/evaluateHealth, status: 400 (Bad Request) response body: } com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException: Status: 400, URL: http://localhost:58192/deployment/evaluateHealth, Message: Bad Request at com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerRetrofitErrorHandler.handleError(SpinnakerRetrofitErrorHandler.java:55) There's also a behavior change for ConversionException. before: 2024-01-04 16:03:19.363 WARN --- [ Test worker] n.s.o.c.t.m.EvaluateDeploymentHealthTask : [] HTTP Error encountered while talking to monitorName(monitorId)->http://localhost:60051/deployment/evaluateHealth, status: 200 (OK) headers: response body: } retrofit.RetrofitError: com.fasterxml.jackson.databind.exc.MismatchedInputException: Cannot construct instance of `com.netflix.spinnaker.orca.deploymentmonitor.models.DeploymentStep` (although at least one Creator exists): no String-argument constructor/factory method to deserialize from String value ('bogus') at [Source: (retrofit.ExceptionCatchingTypedInput$ExceptionCatchingInputStream); line: 1, column: 13] (through reference chain: com.netflix.spinnaker.orca.deploymentmonitor.models.EvaluateHealthResponse["nextStep"]) at retrofit.RetrofitError.conversionError(RetrofitError.java:33) after: 2024-01-04 16:14:03.540 WARN --- [ Test worker] n.s.o.c.t.m.EvaluateDeploymentHealthTask : [] HTTP Error encountered while talking to monitorName(monitorId)->http://localhost:59402/deployment/evaluateHealth, <NO RESPONSE>} com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerConversionException: com.fasterxml.jackson.databind.exc.MismatchedInputException: Cannot construct instance of `com.netflix.spinnaker.orca.deploymentmonitor.models.DeploymentStep` (although at least one Creator exists): no String-argument constructor/factory method to deserialize from String value ('bogus') at [Source: (retrofit.ExceptionCatchingTypedInput$ExceptionCatchingInputStream); line: 1, column: 13] (through reference chain: com.netflix.spinnaker.orca.deploymentmonitor.models.EvaluateHealthResponse["nextStep"]) at com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerRetrofitErrorHandler.handleError(SpinnakerRetrofitErrorHandler.java:64) at retrofit.RestAdapter$RestHandler.invoke(RestAdapter.java:242) --------- Co-authored-by: Pranav-b-7 <[email protected]>

Pranav-b-7 changed the title ~~feat(retrofit): set error handler as SpinnakerRetrofitErrorHandler to simplify the exception handling~~ refactor(retrofit): set error handler as SpinnakerRetrofitErrorHandler to simplify the exception handling Nov 22, 2023

Pranav-b-7 force-pushed the deploymentMonitorService-eliminate-RetrofitError branch from e342423 to 3375c11 Compare November 22, 2023 12:54

Pranav-b-7 marked this pull request as ready for review November 22, 2023 13:09

dbyron-sf reviewed Nov 22, 2023

View reviewed changes