OP-22192 Fixed 100+ test cases during upgrade of orca from 1.30.1 to … #46
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,7 +32,7 @@ dependencies { | |
| } | ||
| implementation("com.amazonaws:aws-java-sdk-lambda") | ||
| implementation("org.bitbucket.b_c:jose4j:0.9.4") | ||
| implementation("jakarta.validation:jakarta.validation-api:3.1.0") | ||
| implementation("com.netflix.frigga:frigga:+") | ||
| implementation("org.jetbrains:annotations") | ||
| implementation("com.fasterxml.jackson.datatype:jackson-datatype-guava") | ||
|
|
@@ -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:1.13.7") { | ||
|
There was a problem hiding this comment. can we pick v1.13.4 from kork. There was a problem hiding this comment. @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. |
||
| force true | ||
| } | ||
| testImplementation("ru.lanwen.wiremock:wiremock-junit5:1.3.1") | ||
|
|
||
| testCompileOnly("org.projectlombok:lombok") | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -111,11 +111,9 @@ class SourceResolver { | |
| regionalAsgs = onlyEnabled | ||
| } | ||
|
|
||
|
There was a problem hiding this comment. this looks simpler & easy to read, why replacing it with your custom logic ? There was a problem hiding this comment. @aman-agrawal @j-sandy Good question. OSS groovy version : 3.0.17..Our version : 4.0.13 After the upgrade we have to write our own comparator logic. Hence the logic was added. This test case checks this logic. you should try out this. |
||
| if (stageData.useSourceCapacity) { | ||
| regionalAsgs = regionalAsgs.sort(false, new CompositeComparator([new InstanceCount(), new CreatedTime()])) | ||
| } | ||
| //with useSourceCapacity prefer the largest ASG over the newest ASG | ||
| def latestAsg = regionalAsgs.last() | ||
| return new StageData.Source( | ||
|
|
@@ -135,4 +133,36 @@ class SourceResolver { | |
| } | ||
| } | ||
|
|
||
| static class InstanceCount implements Comparator<ServerGroup> { | ||
|
There was a problem hiding this comment. how we hav verified that this sorting logic is working fine ? There was a problem hiding this comment. @aman-agrawal @j-sandy Good question. OSS groovy version : 3.0.17..Our version : 4.0.13 After the upgrade we have to write our own comparator logic. Hence the logic was added. This test case checks this logic. you should try out this. |
||
| @Override | ||
| 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. can we ensure o1 and o2 are checked for null values, like : There was a problem hiding this comment. @aman-agrawal @j-sandy Not needed. Code written in accordance to previous versions. Also, i guess, server groups are checked for null before. |
||
| } | ||
| } | ||
|
|
||
| static class CreatedTime implements Comparator<ServerGroup> { | ||
| @Override | ||
| int compare(ServerGroup o1, ServerGroup o2) { | ||
| def createdTime1 = o1.createdTime ?: 0 | ||
| def createdTime2 = o2.createdTime ?: 0 | ||
| createdTime2 <=> createdTime1 | ||
| } | ||
| } | ||
|
|
||
| static class CompositeComparator implements Comparator<ServerGroup> { | ||
| private final List<Comparator<ServerGroup>> comparators | ||
|
|
||
| CompositeComparator(List<Comparator<ServerGroup>> comparators) { | ||
| this.comparators = comparators | ||
| } | ||
|
|
||
| @Override | ||
| int compare(ServerGroup o1, ServerGroup o2) { | ||
| comparators.findResult { it.compare(o1, o2) ?: null } ?: 0 | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -137,19 +137,19 @@ class JarDiffsTask implements DiffTask { | |
| int numberOfInstancesChecked = 0; | ||
| instances.find { key, instanceInfo -> | ||
| if (numberOfInstancesChecked++ >= 5) { | ||
| JarDiffsTask.log.info("Unable to check jar list after 5 attempts, giving up!") | ||
|
There was a problem hiding this comment. Curious : what is JarDiffsTask class doing & why this code change ? There was a problem hiding this comment. @aman-agrawal @j-sandy It is not able to find the log method when running the closure methods. We are getting this exception. JarDiffsTaskSpec > getJarList single server FAILED JarDiffsTaskSpec > getJarList first server fails, second succeeds FAILED JarDiffsTaskSpec > getJarList will short circuit after 5 attempts FAILED JarDiffsTaskSpec > getJarList stops when finds a jar list FAILED |
||
| return true | ||
| } | ||
|
|
||
| String hostName = instanceInfo.privateIpAddress ?: instanceInfo.hostName | ||
| JarDiffsTask.log.debug("attempting to get a jar list from : ${key} (${hostName}:${platformPort})") | ||
| def instanceService = createInstanceService("http://${hostName}:${platformPort}") | ||
| try { | ||
| def instanceResponse = instanceService.getJars() | ||
| jarMap = objectMapper.readValue(instanceResponse.body.in().text, Map) | ||
| return true | ||
| } catch(Exception e) { | ||
| JarDiffsTask.log.debug("could not get a jar list from : ${key} (${hostName}:${platformPort}) - ${e.message}") | ||
| // swallow it so we can try the next instance | ||
| return false | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -51,7 +51,7 @@ class DetermineTerminationCandidatesTask implements Task { | |
| .orElseGet { serverGroupInstances*.instanceId } | ||
| def terminationInstancePool = knownInstanceIds | ||
| if (stage.context.termination?.instances) { | ||
| terminationInstancePool = stage.context.termination?.instances.intersect(knownInstanceIds) | ||
|
There was a problem hiding this comment. can u pls explain this change. There was a problem hiding this comment. @aman-agrawal @j-sandy Good question. OSS groovy version : 3.0.17..Our version : 4.0.13 |
||
| if (stage.context.termination.order == 'given') { | ||
| terminationInstancePool = terminationInstancePool.sort { stage.context.termination.instances.indexOf(it) } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -114,15 +114,15 @@ class V1SchemaIntegrationSpec extends Specification { | |
| test = new IntegrationTest(name: name) | ||
| tests.add(test) | ||
| } | ||
| InputStream inputStream = new FileInputStream(new File(it.path)); | ||
| if (it.filename.endsWith('-config.yml')) { | ||
| test.configuration = objectMapper.convertValue(yaml.load(inputStream), TemplateConfiguration) | ||
| } else if (it.filename.endsWith('-expected.json')) { | ||
| test.expected = objectMapper.readValue(inputStream, Map) | ||
| } else if (it.filename.endsWith('-request.json')) { | ||
| test.request = objectMapper.readValue(inputStream, Map) | ||
| } else { | ||
| test.template.add(objectMapper.convertValue(yaml.load(inputStream), PipelineTemplate)) | ||
|
There was a problem hiding this comment. we are replacing it.file & it.file.text with inputStream, why ? There was a problem hiding this comment. @aman-agrawal @j-sandy Good question. snakeyaml version : 1.27..Our version : 2.0 |
||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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. can we pick v4.0.0 from kork ? There was a problem hiding this comment. @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.
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 |
||
| force = true | ||
| } | ||
| testImplementation(project(":orca-web")) | ||
| testImplementation(project(":orca-queue")) | ||
| testImplementation(project(":orca-clouddriver")) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -36,6 +36,8 @@ import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionType | |
| import com.netflix.spinnaker.orca.config.JedisConfiguration | ||
| import com.netflix.spinnaker.orca.config.RedisConfiguration | ||
| import com.netflix.spinnaker.orca.pipeline.persistence.ExecutionRepository | ||
| import com.netflix.spinnaker.orca.pipeline.tasks.DependsOnExecutionTask | ||
| import com.netflix.spinnaker.orca.pipeline.util.ArtifactUtils | ||
| import com.netflix.spinnaker.orca.q.QueueIntegrationTest | ||
| import com.netflix.spinnaker.orca.q.TestConfig | ||
| import com.netflix.spinnaker.orca.q.migration.ExecutionTypeDeserializer | ||
|
|
@@ -166,6 +168,7 @@ class SqlTestConfig { | |
| @ExtendWith(SpringExtension::class) | ||
| @SpringBootTest( | ||
| classes = [ | ||
| MissingBeanConfiguration::class, | ||
| SqlTestConfig::class, | ||
| SqlProperties::class, | ||
| TestConfig::class, | ||
|
|
@@ -189,3 +192,16 @@ class SqlTestConfig { | |
| ] | ||
| ) | ||
| class SqlQueueIntegrationTest : QueueIntegrationTest() | ||
|
|
||
|
There was a problem hiding this comment. Curious : why we have done this change ? There was a problem hiding this comment. @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 |
||
| @Configuration | ||
| internal class MissingBeanConfiguration { | ||
| @Bean | ||
| fun artifactUtils(): ArtifactUtils { | ||
| return ArtifactUtils(null,null,null) | ||
| } | ||
|
|
||
| @Bean | ||
| fun dependsOnExecutionTask(): DependsOnExecutionTask { | ||
| return DependsOnExecutionTask(null) | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,15 +31,8 @@ import com.netflix.spinnaker.orca.q.CompleteExecution | |
| import com.netflix.spinnaker.orca.q.CompleteStage | ||
| import com.netflix.spinnaker.q.Queue | ||
| import com.netflix.spinnaker.time.fixedClock | ||
| import com.nhaarman.mockito_kotlin.* | ||
| import com.nhaarman.mockito_kotlin.verifyZeroInteractions | ||
| import org.assertj.core.api.Assertions.assertThat | ||
| import org.jetbrains.spek.api.dsl.describe | ||
| import org.jetbrains.spek.api.dsl.given | ||
|
|
@@ -88,8 +81,8 @@ object AbortStageHandlerTest : SubjectSpek<AbortStageHandler>({ | |
| } | ||
|
|
||
| it("does nothing at all") { | ||
| verifyNoMoreInteractions(queue) | ||
|
There was a problem hiding this comment. can we replace verifyZeroInteractions by verifyNoMoreInteractions? There was a problem hiding this comment. @aman-agrawal Already replaced in this file. Did not understand ur comment. There was a problem hiding this comment. why we replaced verifyZeroInteractions by verifyNoMoreInteractions ? There was a problem hiding this comment. Now the question is pertinent enough. There was a problem hiding this comment. VerifyZeroInteractions was deprecated and verifyNoMoreInteractions has the same behaviour. There was a problem hiding this comment. Can u pls verify, It is verifyNoInteractions or verifyNoMoreInteractions ? There was a problem hiding this comment. 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?
|
||
| verifyNoMoreInteractions(publisher) | ||
| verify(repository, never()).storeStage(any()) | ||
| } | ||
| } | ||
|
|
||
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.