test(clouddriver): Test Suite Refinement: MonitoredDeployTask and getRetrofitLogMessage() Behaviour

[Pranav-b-7]

This pull request aims to enhance the testing suite for the MonitoredDeployTask and the getRetrofitLogMessage() method, demonstrating their behavior and preparing for upcoming modifications, which are currently a work in progress under PR : #4614.

Key Changes:

• Test Cases for MonitoredDeployTask: Added test cases to illustrate the behaviour of the MonitoredDeployTask when encountering various RetrofitErrors, including networkError, httpError, unexpectedError, and conversionError.

• Test Cases for getRetrofitLogMessage(): Introduced test cases to validate the behaviour of the getRetrofitLogMessage() method, specifically during the parsing of HTTP error response details such as http headers, http status codes, reason, and http error body. These test cases cover scenarios where exceptions occur during the reading or parsing of these HTTP error details.

Dependency Addition:

• Included the Mockito dependency ('org.mockito:mockito-inline') to enable spying/mocking on the final class retrofit.client.Response. This resolves an issue encountered during testing where Mockito could not mock/spy the final class, leading to the following error:

org.mockito.exceptions.base.MockitoException:
Cannot mock/spy class retrofit.client.Response
Mockito cannot mock/spy because :
final class
at
com.netflix.spinnaker.orca.clouddriver.tasks.monitoreddeploy.MonitoredDeployBaseTaskTest.shouldReturnOnlyStatusWhenExceptionThrownWhileParsingHttpErrorBody(MonitoredDeployBaseTaskTest.java:217)
at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)

[dbyron-sf]

I'm not sure I see the point of all this added test coverage. I mean, I'm all for more test coverage, but if I'm reading the code correctly, EvaluateDeploymentHealthTask doesn't catch any RetrofitErrors (nor have any catch blocks at all), so I don't see how adding SpinnakerRetrofitErrorHandler to DeploymentMonitorService (e.g. in #4614) could change behavior specifically in EvaluateDeploymentHealthTask,

[Pranav-b-7]

@dbyron-sf - This is exactly what I mentioned in the comments : #4614 (comment)

There is no behavioural change . I added this to avoid the confusion we had over the above discussion

[dbyron-sf]

The easiest way to resolve that thread is with some git juggling to move the addition of that one specific test case to a separate commit before the code change in that PR. I suppose these additional tests could be related, but it's hard to see how.

[Pranav-b-7]

These additional test cases are similar, only differences are the type of Spinnaker*Exceptions are different. However if this test cases are confusing / irrelevant , I can still go ahead and remove it.

[dbyron-sf]

I'd rather focus on #4614.

[Pranav-b-7]

Added test case for EvaluateDeploymentTask and moved to a separate commit. : d481b84

[dbyron-sf]

Seems like there's a much simpler way to test this case, e.g.:

  @Test
  void returnsEmptyHttpErrorDetailsWhenExceptionReadingHttpStatus() {

    Response response = mock(Response.class);
    when(response.getStatus()).thenThrow(IllegalArgumentException.class);

    String logMessageOnHttpError = monitoredDeployBaseTask.getRetrofitLogMessage(response);

    assertThat(logMessageOnHttpError).isEqualTo("status: \nheaders: \nresponse body: ");
  }

[Pranav-b-7]

Simplified the test case that verifies the scenario when getRetrofitLogMessage() returns empty http error details.

[dbyron-sf]

same here, e.g.:

  @Test
  void returnsOnlyStatusWhenExceptionParsingHttpErrorBody() {

    Response response = mock(Response.class);
    when(response.getStatus()).thenReturn(HttpStatus.BAD_REQUEST.value()); // arbitrary
    when(response.getReason()).thenReturn("arbitrary reason");
    when(response.getBody()).thenThrow(IllegalArgumentException.class);

    String logMessageOnHttpError = monitoredDeployBaseTask.getRetrofitLogMessage(response);

    String status = String.format("%d (%s)", response.getStatus(), response.getReason());

    assertThat(logMessageOnHttpError).isEqualTo(String.format("status: %s\nheaders: \nresponse body: ", status));
  }

[Pranav-b-7]

Simplified the test case that verifies the scenario when getRetrofitLogMessage() returns only status.

[dbyron-sf]

I haven't worked through this one but I suspect it's similar.

[Pranav-b-7]

Simplified the test case that verifies the scenario when getRetrofitLogMessage() returns only status and body