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

refactor(deploymentmonitor): Configure SpinnakerRetrofitErrorHandler for DeploymentMonitorService #4614

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Pranav-b-7
Copy link
Contributor

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

Changes Made

  • Configured the error handler for the Retrofit client, DeploymentMonitorService, with SpinnakerRetrofitErrorHandler.
  • Modified catch blocks with RetrofitError to use Spinnaker*Exception, aligning with the upcoming Retrofit version upgrade to retrofit2.x.

Purpose

This PR represents foundational work for upgrading the Retrofit version to retrofit2.x, paving the way for future improvements and compatibility.

Behavioural Changes

@@ -45,7 +52,7 @@ class EvaluateDeploymentHealthTaskSpec extends Specification {
given:
Copy link
Contributor

Choose a reason for hiding this comment

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

though underlying exceptions at present are retrofit, wouldn't it be meaningful to change "retrofit errors" to SpinnakerServerExceptions in the test name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thats true.. I have modified the test name to : should retry on SpinnakerServerException

@Pranav-b-7 Pranav-b-7 force-pushed the remove-RetrofitError-DeploymentMonitorService-APIs branch from 4a5ee9a to f2541dc Compare December 8, 2023 15:59
String body =
Optional.ofNullable(objectMapper.writeValueAsString(httpException.getResponseBody()))
.orElse("");
return String.format("headers: %s\nresponse body: %s", httpException.getHeaders(), body);
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 somehow nervous about including an empty body when there might not be one. If the response was present, but wasn't json, I'd like to make that clear somehow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even before this PR, the code would still return empty body headers and status, in case of any parse/read failures.

If we are planning to change this behaviour, I think we have to add a test case before this modifications to demonstrate the behavioural change.

Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't about parse/read failures though. This is about a non-json response. We're guaranteed a behavior change here.

Copy link
Contributor Author

@Pranav-b-7 Pranav-b-7 Jan 2, 2024

Choose a reason for hiding this comment

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

A new specific exception handler is introduced to manage JsonProcessing during the serialization of the HTTP error body.

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 talking about something different. With SpinnakerHttpException, getResponseBody is null if the response isn't json whereas RetrofitError provided access to the response body in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changes added : when the error response body is null, an appropriate error message will be printed in the logger.

} catch (Exception e) {
log.error(
"Failed to fully parse retrofit error while reading response from deployment monitor", e);
"Failed to fully parse http response while reading response from deployment monitor", e);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think there's any parsing here, nor any response reading. We're serializing some json.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Modified the log statement to Failed to fully read http error response details

@@ -198,6 +205,50 @@ class EvaluateDeploymentHealthTaskSpec extends Specification {
false | null || ExecutionStatus.FAILED_CONTINUE
}

def "should return status as RUNNING when SpinnakerHttpException is thrown"() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Possible to add this test before the code change so we can see any change in behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think there is no change in the behaviour here. This test case is to verify if the execute() method returns status as RUNNING when SpinnakerHttpException is thrown. This was the same case even with RetrofitError as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Without having the test before the code change, it's hard to see that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With reference to this comment : #4617 (comment) , moved this test case to a separate commit for a better understanding .

@Pranav-b-7 Pranav-b-7 force-pushed the remove-RetrofitError-DeploymentMonitorService-APIs branch from f2541dc to 53aa376 Compare December 11, 2023 10:41
} catch (Exception e) {
log.error(
"Failed to fully parse retrofit error while reading response from deployment monitor", e);
log.error("Failed to fully read http error response details", e);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
log.error("Failed to fully read http error response details", e);
log.error("Failed to serialize http error response details", e);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The error log is now is now reword to : "Failed to serialize http error response details"

Pranav-b-7 added a commit to Pranav-b-7/orca that referenced this pull request Dec 12, 2023
…RetrofitLogMessage()'

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:2.13.0') has been added to support spying/mocking on the final class . 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 added a commit to Pranav-b-7/orca that referenced this pull request Dec 12, 2023
…RetrofitLogMessage()'

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:2.13.0') 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 added a commit to Pranav-b-7/orca that referenced this pull request Dec 12, 2023
…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:2.13.0') 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 added a commit to Pranav-b-7/orca that referenced this pull request Dec 12, 2023
…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:2.13.0') 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 added a commit to Pranav-b-7/orca that referenced this pull request Dec 12, 2023
…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:2.13.0') 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 added a commit to Pranav-b-7/orca that referenced this pull request Dec 12, 2023
…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:2.13.0') 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 added a commit to Pranav-b-7/orca that referenced this pull request Dec 12, 2023
…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 added a commit to Pranav-b-7/orca that referenced this pull request Dec 12, 2023
…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 added a commit to Pranav-b-7/orca that referenced this pull request Dec 22, 2023
…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 added a commit to Pranav-b-7/orca that referenced this pull request Dec 22, 2023
…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 added a commit to Pranav-b-7/orca that referenced this pull request Dec 26, 2023
…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 remove-RetrofitError-DeploymentMonitorService-APIs branch from cc39981 to 36d3cf0 Compare December 29, 2023 11:45
@Pranav-b-7 Pranav-b-7 closed this Dec 29, 2023
@Pranav-b-7 Pranav-b-7 force-pushed the remove-RetrofitError-DeploymentMonitorService-APIs branch from 36d3cf0 to adc81ac Compare December 29, 2023 12:18
@Pranav-b-7 Pranav-b-7 reopened this Jan 2, 2024
@Pranav-b-7 Pranav-b-7 force-pushed the remove-RetrofitError-DeploymentMonitorService-APIs branch 4 times, most recently from ec0c677 to ce45162 Compare January 2, 2024 11:19
@Pranav-b-7 Pranav-b-7 force-pushed the remove-RetrofitError-DeploymentMonitorService-APIs branch from ce45162 to b681cef Compare January 2, 2024 11:43
Pranav-b-7 added a commit to Pranav-b-7/orca that referenced this pull request Jan 2, 2024
…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 remove-RetrofitError-DeploymentMonitorService-APIs branch from b681cef to 133f1c5 Compare January 2, 2024 12:08
@dbyron-sf dbyron-sf force-pushed the remove-RetrofitError-DeploymentMonitorService-APIs branch from 133f1c5 to d92a982 Compare January 2, 2024 22:57
@@ -198,6 +203,50 @@ class EvaluateDeploymentHealthTaskSpec extends Specification {
false | null || ExecutionStatus.FAILED_CONTINUE
}

def "should return status as RUNNING when Retrofit http error is thrown"() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since the code changes are happening in MonitoredDeployBaseTask, is it possible to add additional coverage to MonitoredDeployBaseTaskTest? Is there anything specific to EvaluateDeploymentHealthTaskSpec that's important?
As well, MonitoredDeployBaseTaskTest is java and I'd much rather add more java code than groovy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The EvaluateDeploymentHealthTask is one of the child class of MonitoredDeployBaseTask .
The executeInternal() method is overridden in this child class , and the there is also a call to evaluateHealth API in this method.
This API call is what we are mocking in the test case. So, I think this test case is more specific to EvaluateDeploymentHealthTask.
Moving this to MonitoredDeployBaseTaskTest would make it more complex , because it requires lots of mocking

@Pranav-b-7 Pranav-b-7 force-pushed the remove-RetrofitError-DeploymentMonitorService-APIs branch 3 times, most recently from 977bd17 to 0ca8d19 Compare January 3, 2024 07:50
@Pranav-b-7 Pranav-b-7 marked this pull request as ready for review January 3, 2024 07:51
This commit introduces a test case aimed at demonstrating the existing behaviour of the ‘EvaluateDeploymentTask’ task. The primary objective of this test case is to verify the proper handling of ‘RetrofitError’ thrown by the retrofit client when interacting with DeploymentMonitorService APIs.

Details:
- Added a dedicated test case to assess the behaviour of ‘EvaluateDeploymentTask’.
- The test case specifically focuses on handling ‘RetrofitError’ instances thrown by DeploymentMonitorService APIs.
- This step is essential for understanding and validating the current behaviour of the task in response to potential errors during API interactions.

This test case serves as a baseline for future modifications and enhancements related to the ‘EvaluateDeploymentTask’.
…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.
@Pranav-b-7 Pranav-b-7 force-pushed the remove-RetrofitError-DeploymentMonitorService-APIs branch from 0ca8d19 to 52e528f Compare January 4, 2024 04:11
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