Skip to content
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.

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

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Pranav-b-7
Copy link
Contributor

@Pranav-b-7 Pranav-b-7 commented Dec 12, 2023

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)

@Pranav-b-7 Pranav-b-7 force-pushed the demonstrate-behaviour-MonitoredDeployTask branch from 53bfc87 to 1192c2c Compare December 12, 2023 14:12
@Pranav-b-7 Pranav-b-7 force-pushed the demonstrate-behaviour-MonitoredDeployTask branch 2 times, most recently from f5a095f to f04e4ef Compare December 12, 2023 16:54
@Pranav-b-7 Pranav-b-7 force-pushed the demonstrate-behaviour-MonitoredDeployTask branch 2 times, most recently from ed03ba2 to f56086a Compare December 12, 2023 17:29
@Pranav-b-7 Pranav-b-7 marked this pull request as ready for review December 13, 2023 06:08
@@ -198,6 +204,202 @@ class EvaluateDeploymentHealthTaskSpec extends Specification {
false | null || ExecutionStatus.FAILED_CONTINUE
}

def "should handle retrofit http error and return the task status depending on the scenarios"() {
Copy link
Contributor

Choose a reason for hiding this comment

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

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,

Copy link
Contributor Author

@Pranav-b-7 Pranav-b-7 Dec 21, 2023

Choose a reason for hiding this comment

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

@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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@Pranav-b-7 Pranav-b-7 Dec 22, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather focus on #4614.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@@ -168,6 +173,99 @@ void shouldReturnDefaultLogMsgWhenUnexpectedErrorHasOccurred() {
assertThat(logMessageOnUnexpectedError).isEqualTo("<NO RESPONSE>");
}

@Test
void shouldReturnEmptyHttpErrorDetailsWhenExceptionThrownWhileReadingHttpStatus() {
Copy link
Contributor

@dbyron-sf dbyron-sf Dec 20, 2023

Choose a reason for hiding this comment

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

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: ");
  }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}

@Test
void shouldReturnOnlyStatusWhenExceptionThrownWhileParsingHttpErrorBody() {
Copy link
Contributor

Choose a reason for hiding this comment

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

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));
  }

Copy link
Contributor Author

@Pranav-b-7 Pranav-b-7 Dec 22, 2023

Choose a reason for hiding this comment

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

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


@Test
void shouldReturnOnlyStatusAndBodyWhenExceptionThrownWhileReadingHttpHeaders()
throws IOException {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@Pranav-b-7 Pranav-b-7 force-pushed the demonstrate-behaviour-MonitoredDeployTask branch 2 times, most recently from f8a4f43 to 42d18e1 Compare December 22, 2023 06:39
@Pranav-b-7 Pranav-b-7 force-pushed the demonstrate-behaviour-MonitoredDeployTask branch from 42d18e1 to e114897 Compare December 26, 2023 04:04
Pranav-b-7 added a commit to Pranav-b-7/orca that referenced this pull request Jan 2, 2024
…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
…DeployTask and 'getRetrofitLogMessage()'

This commit sets the stage for forthcoming changes currently in progress under PR : spinnaker#4614 . The primary goal is to compare the behaviour before and after enhancements by introducing test cases for the ‘MonitoredDeployTask’ and the ‘getRetrofitLogMessage()’ method.

For ‘MonitoredDeployTask’:
- Test Case to simulate networkError and observe behaviour
- Test Case to simulate httpError and observe behaviour
- Test Case to simulate unexpectedError and observe behaviour
- Test Case to simulate conversionError and observe behaviour

For ‘getRetrofitLogMessage()’:
- Test Cases to verify behaviour during HTTP error details parsing when exceptions occur

Additionally, a Mockito dependency ('org.mockito:mockito-inline') has been added to support spying/mocking on the final class 'retrofit.client.Response'. This resolves the issue encountered during testing where Mockito couldn't mock/spy the final class, preventing 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)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:568)
	at org.junit.platform.commons.util.ReflectionUtils.invokeMethod(ReflectionUtils.java:688)
	at org.junit.jupiter.engine.execution.MethodInvocation.proceed(MethodInvocation.java:60)
	at org.junit.jupiter.engine.execution.InvocationInterceptorChain.proceed(InvocationInterceptorChain.java:131)
@Pranav-b-7 Pranav-b-7 force-pushed the demonstrate-behaviour-MonitoredDeployTask branch from e114897 to 43964ce Compare January 2, 2024 12:08
Pranav-b-7 added a commit to Pranav-b-7/orca that referenced this pull request Jan 2, 2024
…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
dbyron-sf pushed a commit to Pranav-b-7/orca that referenced this pull request Jan 2, 2024
…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
Pranav-b-7 added a commit to Pranav-b-7/orca that referenced this pull request Jan 3, 2024
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants