-
Notifications
You must be signed in to change notification settings - Fork 6
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
OP-22192 Fixed 100+ test cases during upgrade of orca from 1.30.1 to … #46
Conversation
@@ -88,8 +81,8 @@ object AbortStageHandlerTest : SubjectSpek<AbortStageHandler>({ | |||
} | |||
|
|||
it("does nothing at all") { | |||
verifyZeroInteractions(queue) | |||
verifyZeroInteractions(publisher) | |||
verifyNoMoreInteractions(queue) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we replace verifyZeroInteractions by verifyNoMoreInteractions?
Applicable elsewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aman-agrawal Already replaced in this file. Did not understand ur comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why we replaced verifyZeroInteractions by verifyNoMoreInteractions ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now the question is pertinent enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
VerifyZeroInteractions was deprecated and verifyNoMoreInteractions has the same behaviour.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can u pls verify, It is verifyNoInteractions or verifyNoMoreInteractions ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is where i respectfully differ. You should do your homework....I am explaining it for now...next time cannot expect the same from me....Both are same. Little lazy with my writing skills... 🤣
Now what do you think? Should i be using verifyNoMoreInteractions or verifyNoInteractions?
import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionStatus.NOT_STARTED | ||
import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionStatus.SUCCEEDED | ||
import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionStatus.TERMINAL | ||
import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionStatus.* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pls import specific classes. Applicable elsewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah will do it.
@@ -33,7 +33,9 @@ dependencies { | |||
api("org.springframework.boot:spring-boot-starter-test") | |||
api("org.springframework.security:spring-security-test") | |||
api("org.springframework.security:spring-security-config") | |||
api("com.ninja-squad:springmockk:+") | |||
api("com.ninja-squad:springmockk:4.0.2") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we pick v4.0.0 from kork.
if not, any specific reason to use 4.0.2 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aman-agrawal @j-sandy No specific reason to use 4.0.2. We can use 4.0.0 also.
i had to force this dependency , otherwise this dependency version is set to 2.0.3 and the test case fails.
@@ -58,7 +58,9 @@ dependencies { | |||
testImplementation("io.spinnaker.fiat:fiat-core:$fiatVersion") | |||
testImplementation("dev.minutest:minutest") | |||
testImplementation("io.strikt:strikt-core") | |||
testImplementation("io.mockk:mockk") | |||
testImplementation("io.mockk:mockk:1.13.7") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we pick v1.13.4 from kork.
if not, any specific reason to use 1.13.7 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aman-agrawal @j-sandy No specific reason to use 1.13.7. We can use 1.13.4 also.
i had to force this dependency , otherwise this dependency version is set to 1.10.5 and the test case fails.
@@ -137,19 +137,19 @@ class JarDiffsTask implements DiffTask { | |||
int numberOfInstancesChecked = 0; | |||
instances.find { key, instanceInfo -> | |||
if (numberOfInstancesChecked++ >= 5) { | |||
log.info("Unable to check jar list after 5 attempts, giving up!") | |||
JarDiffsTask.log.info("Unable to check jar list after 5 attempts, giving up!") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious : what is JarDiffsTask class doing & why this code change ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aman-agrawal @j-sandy It is not able to find the log method when running the closure methods. We are getting this exception.
When added the classname to the log object,all the test cases are passing.
JarDiffsTaskSpec > getJarList single server FAILED
groovy.lang.MissingMethodException: No signature of method: com.netflix.spinnaker.orca.kato.tasks.JarDiffsTask.getLog() is applicable for argument types: () values: []
Possible solutions: getAt(java.lang.String), getClass(), notify(), $getLookup(), every(), grep()
at com.netflix.spinnaker.orca.kato.tasks.JarDiffsTask.getJarList_closure2(JarDiffsTask.groovy:145)
at app//com.netflix.spinnaker.orca.kato.tasks.JarDiffsTask.getJarList(JarDiffsTask.groovy:138)
at app//org.spockframework.mock.runtime.ByteBuddyMethodInvoker.respond(ByteBuddyMethodInvoker.java:20)
at app//org.spockframework.mock.runtime.MockInvocation.callRealMethod(MockInvocation.java:61)
at app//org.spockframework.mock.CallRealMethodResponse.respond(CallRealMethodResponse.java:30)
at app//org.spockframework.mock.runtime.MockController.handle(MockController.java:52)
at app//org.spockframework.mock.runtime.JavaMockInterceptor.intercept(JavaMockInterceptor.java:84)
at app//org.spockframework.mock.runtime.ByteBuddyInterceptorAdapter.interceptNonAbstract(ByteBuddyInterceptorAdapter.java:35)
at com.netflix.spinnaker.orca.kato.tasks.JarDiffsTaskSpec.getJarList single server(JarDiffsTaskSpec.groovy:87)
JarDiffsTaskSpec > getJarList first server fails, second succeeds FAILED
groovy.lang.MissingMethodException: No signature of method: com.netflix.spinnaker.orca.kato.tasks.JarDiffsTask.getLog() is applicable for argument types: () values: []
Possible solutions: getAt(java.lang.String), getClass(), notify(), $getLookup(), every(), grep()
at com.netflix.spinnaker.orca.kato.tasks.JarDiffsTask.getJarList_closure2(JarDiffsTask.groovy:145)
at app//com.netflix.spinnaker.orca.kato.tasks.JarDiffsTask.getJarList(JarDiffsTask.groovy:138)
at app//org.spockframework.mock.runtime.ByteBuddyMethodInvoker.respond(ByteBuddyMethodInvoker.java:20)
at app//org.spockframework.mock.runtime.MockInvocation.callRealMethod(MockInvocation.java:61)
at app//org.spockframework.mock.CallRealMethodResponse.respond(CallRealMethodResponse.java:30)
at app//org.spockframework.mock.runtime.MockController.handle(MockController.java:52)
at app//org.spockframework.mock.runtime.JavaMockInterceptor.intercept(JavaMockInterceptor.java:84)
at app//org.spockframework.mock.runtime.ByteBuddyInterceptorAdapter.interceptNonAbstract(ByteBuddyInterceptorAdapter.java:35)
at com.netflix.spinnaker.orca.kato.tasks.JarDiffsTaskSpec.getJarList first server fails, second succeeds(JarDiffsTaskSpec.groovy:109)
JarDiffsTaskSpec > getJarList will short circuit after 5 attempts FAILED
groovy.lang.MissingMethodException: No signature of method: com.netflix.spinnaker.orca.kato.tasks.JarDiffsTask.getLog() is applicable for argument types: () values: []
Possible solutions: getAt(java.lang.String), getClass(), notify(), $getLookup(), every(), grep()
at com.netflix.spinnaker.orca.kato.tasks.JarDiffsTask.getJarList_closure2(JarDiffsTask.groovy:145)
at app//com.netflix.spinnaker.orca.kato.tasks.JarDiffsTask.getJarList(JarDiffsTask.groovy:138)
at app//org.spockframework.mock.runtime.ByteBuddyMethodInvoker.respond(ByteBuddyMethodInvoker.java:20)
at app//org.spockframework.mock.runtime.MockInvocation.callRealMethod(MockInvocation.java:61)
at app//org.spockframework.mock.CallRealMethodResponse.respond(CallRealMethodResponse.java:30)
at app//org.spockframework.mock.runtime.MockController.handle(MockController.java:52)
at app//org.spockframework.mock.runtime.JavaMockInterceptor.intercept(JavaMockInterceptor.java:84)
at app//org.spockframework.mock.runtime.ByteBuddyInterceptorAdapter.interceptNonAbstract(ByteBuddyInterceptorAdapter.java:35)
at com.netflix.spinnaker.orca.kato.tasks.JarDiffsTaskSpec.getJarList will short circuit after 5 attempts(JarDiffsTaskSpec.groovy:126)
JarDiffsTaskSpec > getJarList stops when finds a jar list FAILED
groovy.lang.MissingMethodException: No signature of method: com.netflix.spinnaker.orca.kato.tasks.JarDiffsTask.getLog() is applicable for argument types: () values: []
Possible solutions: getAt(java.lang.String), getClass(), notify(), $getLookup(), every(), grep()
at com.netflix.spinnaker.orca.kato.tasks.JarDiffsTask.getJarList_closure2(JarDiffsTask.groovy:145)
at app//com.netflix.spinnaker.orca.kato.tasks.JarDiffsTask.getJarList(JarDiffsTask.groovy:138)
at app//org.spockframework.mock.runtime.ByteBuddyMethodInvoker.respond(ByteBuddyMethodInvoker.java:20)
at app//org.spockframework.mock.runtime.MockInvocation.callRealMethod(MockInvocation.java:61)
at app//org.spockframework.mock.CallRealMethodResponse.respond(CallRealMethodResponse.java:30)
at app//org.spockframework.mock.runtime.MockController.handle(MockController.java:52)
at app//org.spockframework.mock.runtime.JavaMockInterceptor.intercept(JavaMockInterceptor.java:84)
at app//org.spockframework.mock.runtime.ByteBuddyInterceptorAdapter.interceptNonAbstract(ByteBuddyInterceptorAdapter.java:35)
at com.netflix.spinnaker.orca.kato.tasks.JarDiffsTaskSpec.getJarList stops when finds a jar list(JarDiffsTaskSpec.groovy:144)
int compare(ServerGroup o1, ServerGroup o2) { | ||
def size1 = o1.instances?.size() ?: 0 | ||
def size2 = o2.instances?.size() ?: 0 | ||
size1 <=> size2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we ensure o1 and o2 are checked for null values, like : o1?.instances?.size() ?: 0
Applicable elsewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aman-agrawal @j-sandy Not needed. Code written in accordance to previous versions. Also, i guess, server groups are checked for null before.
@@ -51,7 +51,7 @@ class DetermineTerminationCandidatesTask implements Task { | |||
.orElseGet { serverGroupInstances*.instanceId } | |||
def terminationInstancePool = knownInstanceIds | |||
if (stage.context.termination?.instances) { | |||
terminationInstancePool = knownInstanceIds.intersect(stage.context.termination?.instances) | |||
terminationInstancePool = stage.context.termination?.instances.intersect(knownInstanceIds) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can u pls explain this change.
why we are making this ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aman-agrawal @j-sandy Good question. OSS groovy version : 3.0.17..Our version : 4.0.13
intersect method functionality changed. We can create a sample project and analyse for yourselves.
} else { | ||
test.template.add(objectMapper.convertValue(yaml.load(it.file.text), PipelineTemplate)) | ||
test.template.add(objectMapper.convertValue(yaml.load(inputStream), PipelineTemplate)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we are replacing it.file & it.file.text with inputStream, why ?
can you pls explain this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aman-agrawal @j-sandy Good question. snakeyaml version : 1.27..Our version : 2.0
load method functionality changed. We can create a sample project and analyse for yourselves.
@@ -18,7 +18,9 @@ apply from: "$rootDir/gradle/kotlin.gradle" | |||
|
|||
dependencies { | |||
implementation(project(":orca-api")) | |||
|
|||
api("com.ninja-squad:springmockk:4.0.2") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we pick v4.0.0 from kork ?
if not, any specific reason for this version ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aman-agrawal @j-sandy No specific reason to use 4.0.2. We can use 4.0.0 also.
i had to force this dependency , otherwise this dependency version is set to 2.0.3 and the test case fails.
Task :orca-plugins-test:test
OrcaPluginsTest > minutests() > an orca integration test environment and an orca plugin > an integration test environment and relevant plugins > com.netflix.spinnaker.orca.plugins.test.OrcaPluginsTest.minutests()[1][1][1] FAILED
java.lang.NoClassDefFoundError: org/springframework/beans/factory/config/InstantiationAwareBeanPostProcessorAdapter
at java.base/java.lang.ClassLoader.defineClass1(Native Method)
at java.base/java.lang.ClassLoader.defineClass(ClassLoader.java:1017)
at java.base/java.security.SecureClassLoader.defineClass(SecureClassLoader.java:150)
at java.base/jdk.internal.loader.BuiltinClassLoader.defineClass(BuiltinClassLoader.java:862)
at java.base/jdk.internal.loader.BuiltinClassLoader.findClassOnClassPathOrNull(BuiltinClassLoader.java:760)
at java.base/jdk.internal.loader.BuiltinClassLoader.loadClassOrNull(BuiltinClassLoader.java:681)
at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:639)
at java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:188)
at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:525)
at com.ninjasquad.springmockk.MockkContextCustomizer.customizeContext(MockkContextCustomizer.kt:21)
at org.springframework.boot.test.context.SpringBootContextLoader$ContextCustomizerAdapter.initialize(SpringBootContextLoader.java:435)
at org.springframework.boot.SpringApplication.applyInitializers(SpringApplication.java:605)
at org.springframework.boot.SpringApplication.prepareContext(SpringApplication.java:385)
at org.springframework.boot.SpringApplication.run(SpringAppli
@@ -189,3 +192,16 @@ class SqlTestConfig { | |||
] | |||
) | |||
class SqlQueueIntegrationTest : QueueIntegrationTest() | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious : why we have done this change ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aman-agrawal @j-sandy ..Because of the kotlin upgrade, the test container does not get started. reason being missing beans(artifactUtils, dependsOnExecutionTask)...and we have to provide just at the start of the test case.
Hence, the change
Jira : https://devopsmx.atlassian.net/browse/OP-22192
Testing : Compile success, 1 TCs failed bcoz of the bean creation pos processor methods deployed in local and the service started successfully.
Note :
All commits covered - verified from sheet.