From a1ce5e87c641249b2f1ce300903ab836dbfaa91c Mon Sep 17 00:00:00 2001
From: Robbie McKinstry <thesnowmancometh@gmail.com>
Date: Mon, 18 Nov 2024 12:46:08 -0500
Subject: [PATCH] Fix bug when advancing stages.

This commit fixes a bug that always advanced stages to the end
of the list. It adds a unit test to ensure the behavior remains fixed.
This is exactly the value of unit tests -- even something I thought
was a fairly straight-forward implementation and a near-tautological
test turned out to be valuable in rooting out a bug.
---
 src/pipeline/stages/config.rs | 37 +++++++++++++++++++++++++++++++----
 1 file changed, 33 insertions(+), 4 deletions(-)

diff --git a/src/pipeline/stages/config.rs b/src/pipeline/stages/config.rs
index 136b03a..517c649 100644
--- a/src/pipeline/stages/config.rs
+++ b/src/pipeline/stages/config.rs
@@ -5,6 +5,8 @@ use crate::pipeline::percent::{DecimalPercent, WholePercent};
 use super::details::StageDetails;
 
 const DEFAULT_STAGE_TIMEOUT: Duration = Duration::from_secs(5 * 60);
+const DEFAULT_CANARY_STAGE_TRAFFIC: [u8; 4] = [5, 20, 40, 60];
+const DEFAULT_CANARY_STAGE_CONFIDENCE: [f64; 4] = [99.0, 95.0, 90.0, 90.0];
 
 /// A description of the stages in this deployment pipeline.
 pub struct StageConfig {
@@ -18,11 +20,11 @@ impl StageConfig {
         // of the vector. Use the max function to ensure we never
         // go too far afield from the edge of the vector in case
         // we have to rollback by one stage.
-        self.current_stage = std::cmp::max(self.current_stage + 1, self.stages.len());
+        self.current_stage = std::cmp::min(self.current_stage + 1, self.stages.len());
         self.current()
     }
 
-    fn current(&mut self) -> Option<&StageDetails> {
+    fn current(&self) -> Option<&StageDetails> {
         self.stages.get(self.current_stage)
     }
 }
@@ -35,12 +37,12 @@ impl Default for StageConfig {
         let mut stages = vec![];
         // Declare the amounts of traffic the canary should get
         // at each stage.
-        let stage_traffics = [5, 20, 40, 60]
+        let stage_traffics = DEFAULT_CANARY_STAGE_TRAFFIC
             .into_iter()
             .map(|val| WholePercent::try_new(val).unwrap());
         // Declare the amount of confidence we need to have in
         // the badness of the deploy before we rollback.
-        let stage_confidence = [99.0, 95.0, 90.0, 90.0]
+        let stage_confidence = DEFAULT_CANARY_STAGE_CONFIDENCE
             .into_iter()
             .map(|val| DecimalPercent::try_new(val).unwrap());
         // Create the stages one at a time, adding them to the array.
@@ -60,3 +62,30 @@ impl Default for StageConfig {
         }
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use crate::pipeline::stages::config::{
+        DEFAULT_CANARY_STAGE_CONFIDENCE, DEFAULT_CANARY_STAGE_TRAFFIC,
+    };
+
+    use super::StageConfig;
+
+    /// This test demonstrates that we can advance through the default stages
+    /// and read their values.
+    #[test]
+    fn advance_default_stages() {
+        let mut conf = StageConfig::default();
+        let mut stage = conf.current();
+        for i in 0..4 {
+            let expected_traffic = Some(DEFAULT_CANARY_STAGE_TRAFFIC[i]);
+            let observed_traffic = stage.map(|st| st.canary_traffic());
+            let expected_confidence = Some(DEFAULT_CANARY_STAGE_CONFIDENCE[i]);
+            let observed_confidence = stage.map(|st| st.badness_confidence_limit());
+            assert_eq!(expected_traffic, observed_traffic, "On iteration {i}, expected traffic {expected_traffic:?} but observed {observed_traffic:?}");
+            assert_eq!(expected_confidence, observed_confidence, "On iteration {i}, expected confidence {expected_confidence:?} but observed {observed_confidence:?}" );
+            stage = conf.advance();
+        }
+        assert!(stage.is_none());
+    }
+}