From a1cc8bfbfbbe97856088730b5001d4ed6e85eaa3 Mon Sep 17 00:00:00 2001 From: Daniel Zheng Date: Tue, 15 Aug 2023 13:14:03 -0700 Subject: [PATCH 1/2] test(handler): Verify that start time is set Verify that the start time for a stage is set even when we encounter an exception --- .../orca/q/handler/StartStageHandlerTest.kt | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/orca-queue/src/test/kotlin/com/netflix/spinnaker/orca/q/handler/StartStageHandlerTest.kt b/orca-queue/src/test/kotlin/com/netflix/spinnaker/orca/q/handler/StartStageHandlerTest.kt index ae5ded7972..d82c291c53 100644 --- a/orca-queue/src/test/kotlin/com/netflix/spinnaker/orca/q/handler/StartStageHandlerTest.kt +++ b/orca-queue/src/test/kotlin/com/netflix/spinnaker/orca/q/handler/StartStageHandlerTest.kt @@ -799,6 +799,14 @@ object StartStageHandlerTest : SubjectSpek({ } ) } + + it("updates the stage with a non-default start time") { + verify(repository).storeStage( + check { + assertThat(it.startTime).isPositive() + } + ) + } } and("only the branch should fail") { @@ -839,6 +847,14 @@ object StartStageHandlerTest : SubjectSpek({ } ) } + + it("updates the stage with a non-default start time") { + verify(repository).storeStage( + check { + assertThat(it.startTime).isPositive() + } + ) + } } and("the branch should be allowed to continue") { @@ -879,6 +895,14 @@ object StartStageHandlerTest : SubjectSpek({ } ) } + + it("updates the stage with a non-default start time") { + verify(repository).storeStage( + check { + assertThat(it.startTime).isPositive() + } + ) + } } } From e566e21094b435c32228cbf77d731316ab0896c0 Mon Sep 17 00:00:00 2001 From: Daniel Zheng Date: Mon, 14 Aug 2023 16:27:47 -0700 Subject: [PATCH 2/2] perf(stage): Remove duplicate storeStage call The duplicate repository.storeStage call was originally added in https://github.com/spinnaker/orca/commit/e872ce859e5e82ca9a4726bea596ae1513b0e765 to ensure that the start time for a stage is set even if the handler encounters an exception. However, even without the extra repository.storeStage, orca still sets the start time for a stage when it encounters an exception. --- .../orca/q/handler/StartStageHandler.kt | 1 - .../orca/q/handler/StartStageHandlerTest.kt | 28 +++++++++---------- 2 files changed, 14 insertions(+), 15 deletions(-) diff --git a/orca-queue/src/main/kotlin/com/netflix/spinnaker/orca/q/handler/StartStageHandler.kt b/orca-queue/src/main/kotlin/com/netflix/spinnaker/orca/q/handler/StartStageHandler.kt index 06e9af2bb2..3a18214ff1 100644 --- a/orca-queue/src/main/kotlin/com/netflix/spinnaker/orca/q/handler/StartStageHandler.kt +++ b/orca-queue/src/main/kotlin/com/netflix/spinnaker/orca/q/handler/StartStageHandler.kt @@ -95,7 +95,6 @@ class StartStageHandler( try { // Set the startTime in case we throw an exception. stage.startTime = clock.millis() - repository.storeStage(stage) stage.plan() stage.status = RUNNING repository.storeStage(stage) diff --git a/orca-queue/src/test/kotlin/com/netflix/spinnaker/orca/q/handler/StartStageHandlerTest.kt b/orca-queue/src/test/kotlin/com/netflix/spinnaker/orca/q/handler/StartStageHandlerTest.kt index d82c291c53..adb4886e2c 100644 --- a/orca-queue/src/test/kotlin/com/netflix/spinnaker/orca/q/handler/StartStageHandlerTest.kt +++ b/orca-queue/src/test/kotlin/com/netflix/spinnaker/orca/q/handler/StartStageHandlerTest.kt @@ -162,7 +162,7 @@ object StartStageHandlerTest : SubjectSpek({ } it("updates the stage status") { - verify(repository, times(2)).storeStage( + verify(repository).storeStage( check { assertThat(it.status).isEqualTo(RUNNING) assertThat(it.startTime).isEqualTo(clock.millis()) @@ -171,7 +171,7 @@ object StartStageHandlerTest : SubjectSpek({ } it("attaches tasks to the stage") { - verify(repository, times(2)).storeStage( + verify(repository).storeStage( check { assertThat(it.tasks.size).isEqualTo(1) it.tasks.first().apply { @@ -222,7 +222,7 @@ object StartStageHandlerTest : SubjectSpek({ } it("updates the stage status") { - verify(repository, times(2)).storeStage( + verify(repository).storeStage( check { assertThat(it.status).isEqualTo(RUNNING) assertThat(it.startTime).isEqualTo(clock.millis()) @@ -267,7 +267,7 @@ object StartStageHandlerTest : SubjectSpek({ } it("updates the stage status") { - verify(repository, times(2)).storeStage( + verify(repository).storeStage( check { assertThat(it.status).isEqualTo(RUNNING) assertThat(it.startTime).isEqualTo(clock.millis()) @@ -312,7 +312,7 @@ object StartStageHandlerTest : SubjectSpek({ afterGroup(::resetMocks) it("attaches tasks to the stage") { - verify(repository, times(2)).storeStage( + verify(repository).storeStage( check { assertThat(it.tasks.size).isEqualTo(3) it.tasks[0].apply { @@ -702,7 +702,7 @@ object StartStageHandlerTest : SubjectSpek({ } it("starts the stage") { - verify(repository, times(2)).storeStage( + verify(repository).storeStage( check { assertThat(it.type).isEqualTo("bar") assertThat(it.status).isEqualTo(RUNNING) @@ -712,7 +712,7 @@ object StartStageHandlerTest : SubjectSpek({ } it("attaches a task to the stage") { - verify(repository, times(2)).storeStage( + verify(repository).storeStage( check { assertThat(it.tasks.size).isEqualTo(1) it.tasks.first().apply { @@ -793,7 +793,7 @@ object StartStageHandlerTest : SubjectSpek({ } it("attaches the exception to the stage context") { - verify(repository, times(2)).storeStage( + verify(repository).storeStage( check { assertThat(it.context["exception"]).isEqualTo(exceptionDetails) } @@ -833,7 +833,7 @@ object StartStageHandlerTest : SubjectSpek({ } it("attaches the exception to the stage context") { - verify(repository, times(2)).storeStage( + verify(repository).storeStage( check { assertThat(it.context["exception"]).isEqualTo(exceptionDetails) } @@ -841,7 +841,7 @@ object StartStageHandlerTest : SubjectSpek({ } it("attaches flag to the stage context to indicate that before stage planning failed") { - verify(repository, times(2)).storeStage( + verify(repository).storeStage( check { assertThat(it.context["beforeStagePlanningFailed"]).isEqualTo(true) } @@ -881,7 +881,7 @@ object StartStageHandlerTest : SubjectSpek({ } it("attaches the exception to the stage context") { - verify(repository, times(2)).storeStage( + verify(repository).storeStage( check { assertThat(it.context["exception"]).isEqualTo(exceptionDetails) } @@ -889,7 +889,7 @@ object StartStageHandlerTest : SubjectSpek({ } it("attaches flag to the stage context to indicate that before stage planning failed") { - verify(repository, times(2)).storeStage( + verify(repository).storeStage( check { assertThat(it.context["beforeStagePlanningFailed"]).isEqualTo(true) } @@ -953,7 +953,7 @@ object StartStageHandlerTest : SubjectSpek({ } it("updates the stage status") { - verify(repository, times(2)).storeStage( + verify(repository).storeStage( check { assertThat(it.status).isEqualTo(RUNNING) assertThat(it.startTime).isEqualTo(clock.millis()) @@ -962,7 +962,7 @@ object StartStageHandlerTest : SubjectSpek({ } it("attaches tasks to the stage") { - verify(repository, times(2)).storeStage( + verify(repository).storeStage( check { assertThat(it.tasks.size).isEqualTo(1) it.tasks.first().apply {