Merge pull request #2352 from input-output-hk/sfa/fix_flakiness_on_should_delay_between_retries_tests

Remove flakiness on should delay between retries tests

Author: sfauvel

Cargo.lock

@@ -1,6 +1,6 @@
 [package]
 name = "mithril-aggregator"
-version = "0.7.11"
+version = "0.7.12"
 description = "A Mithril Aggregator server"
 authors = { workspace = true }
 edition = { workspace = true }

mithril-aggregator/Cargo.toml

@@ -67,7 +67,7 @@ pub trait FileUploader: Sync + Send {
 
 #[cfg(test)]
 mod tests {
-    use std::{path::PathBuf, time::Instant};
+    use std::{path::PathBuf, sync::Mutex, time::Instant};
 
     use super::*;
     use anyhow::anyhow;
@@ -172,39 +172,53 @@ mod tests {
 
     #[tokio::test]
     async fn should_delay_between_retries() {
-        let mut uploader = MockTestFileUploader::new();
+        struct FileUploaderAssertDelay {
+            delay_ms: u64,
+            last_attempt_start_time: Mutex<Option<Instant>>,
+        }
 
-        let delay = Duration::from_millis(50);
-        uploader
-            .expect_retry_policy()
-            .returning(move || FileUploadRetryPolicy {
-                attempts: 4,
-                delay_between_attempts: delay,
-            });
+        #[async_trait]
+        impl FileUploader for FileUploaderAssertDelay {
+            async fn upload_without_retry(&self, _filepath: &Path) -> StdResult<FileUri> {
+                let mut last_attempt_start_time = self.last_attempt_start_time.lock().unwrap();
+                if let Some(last_start_attempt) = *last_attempt_start_time {
+                    let duration = last_start_attempt.elapsed();
+                    let expected_delay_greater_than_or_equal = Duration::from_millis(self.delay_ms);
+                    assert!(
+                        duration >= expected_delay_greater_than_or_equal,
+                        "duration should be greater than or equal to {}ms but was {}ms",
+                        expected_delay_greater_than_or_equal.as_millis(),
+                        duration.as_millis()
+                    );
+                    let expected_delay_less_than = Duration::from_millis(2 * self.delay_ms);
+                    assert!(
+                        duration < expected_delay_less_than,
+                        "duration should be less than {}ms but was {}ms",
+                        expected_delay_less_than.as_millis(),
+                        duration.as_millis()
+                    );
+                }
+                *last_attempt_start_time = Some(Instant::now());
 
-        uploader
-            .expect_upload_without_retry()
-            .times(4)
-            .returning(move |_| Err(anyhow!("Failure while uploading...")));
+                Err(anyhow::anyhow!("Upload failed"))
+            }
+
+            fn retry_policy(&self) -> FileUploadRetryPolicy {
+                FileUploadRetryPolicy {
+                    attempts: 4,
+                    delay_between_attempts: Duration::from_millis(self.delay_ms),
+                }
+            }
+        }
+
+        let uploader = FileUploaderAssertDelay {
+            delay_ms: 50,
+            last_attempt_start_time: Mutex::new(None),
+        };
 
-        let start = Instant::now();
         uploader
             .upload(&PathBuf::from("file_to_upload"))
             .await
             .expect_err("An error should be returned when all retries are done");
-        let duration = start.elapsed();
-
-        assert!(
-            duration >= delay * 3,
-            "Duration should be at least 3 times the delay ({}ms) but was {}ms",
-            delay.as_millis() * 3,
-            duration.as_millis()
-        );
-        assert!(
-            duration < delay * 4,
-            "Duration should be less than 4 times the delay ({}ms) but was {}ms",
-            delay.as_millis() * 4,
-            duration.as_millis()
-        );
     }
 }

mithril-aggregator/src/file_uploaders/interface.rs

@@ -1,6 +1,6 @@
 [package]
 name = "mithril-client"
-version = "0.11.11"
+version = "0.11.12"
 description = "Mithril client library"
 authors = { workspace = true }
 edition = { workspace = true }

mithril-client/Cargo.toml

@@ -97,7 +97,7 @@ impl FileDownloader for RetryDownloader {
 #[cfg(test)]
 mod tests {
 
-    use std::time::Instant;
+    use std::{sync::Mutex, time::Instant};
 
     use mithril_common::entities::FileUri;
 
@@ -227,22 +227,59 @@ mod tests {
 
     #[tokio::test]
     async fn should_delay_between_retries() {
-        let mock_file_downloader = MockFileDownloaderBuilder::default()
-            .with_file_uri("http://whatever/00001.tar.gz")
-            .with_compression(None)
-            .with_failure()
-            .with_times(4)
-            .build();
-        let delay = Duration::from_millis(50);
+        struct FileDownloaderAssertDelay {
+            expected_delay_greater_than_or_equal: Duration,
+            expected_delay_less_than: Duration,
+            last_attempt_start_time: Mutex<Option<Instant>>,
+        }
+
+        #[async_trait]
+        impl FileDownloader for FileDownloaderAssertDelay {
+            async fn download_unpack(
+                &self,
+                _location: &FileDownloaderUri,
+                _file_size: u64,
+                _target_dir: &Path,
+                _compression_algorithm: Option<CompressionAlgorithm>,
+                _download_event_type: DownloadEvent,
+            ) -> StdResult<()> {
+                let mut last_attempt_start_time = self.last_attempt_start_time.lock().unwrap();
+                if let Some(last_start_attempt) = *last_attempt_start_time {
+                    let duration = last_start_attempt.elapsed();
+                    assert!(
+                        duration >= self.expected_delay_greater_than_or_equal,
+                        "duration should be greater than or equal to {}ms but was {}ms",
+                        self.expected_delay_greater_than_or_equal.as_millis(),
+                        duration.as_millis()
+                    );
+                    assert!(
+                        duration < self.expected_delay_less_than,
+                        "duration should be less than {}ms but was {}ms",
+                        self.expected_delay_less_than.as_millis(),
+                        duration.as_millis()
+                    );
+                }
+                *last_attempt_start_time = Some(Instant::now());
+
+                Err(anyhow::anyhow!("Download failed"))
+            }
+        }
+
+        let delay_ms = 50;
+        let mock_file_downloader = Arc::new(FileDownloaderAssertDelay {
+            expected_delay_greater_than_or_equal: Duration::from_millis(delay_ms),
+            expected_delay_less_than: Duration::from_millis(2 * delay_ms),
+            last_attempt_start_time: Mutex::new(None),
+        });
+
         let retry_downloader = RetryDownloader::new(
-            Arc::new(mock_file_downloader),
+            mock_file_downloader.clone(),
             FileDownloadRetryPolicy {
                 attempts: 4,
-                delay_between_attempts: delay,
+                delay_between_attempts: Duration::from_millis(delay_ms),
             },
         );
 
-        let start = Instant::now();
         retry_downloader
             .download_unpack(
                 &FileDownloaderUri::FileUri(FileUri("http://whatever/00001.tar.gz".to_string())),
@@ -256,19 +293,5 @@ mod tests {
             )
             .await
             .expect_err("An error should be returned when all download attempts fail");
-        let duration = start.elapsed();
-
-        assert!(
-            duration >= delay * 3,
-            "Duration should be at least 3 times the delay ({}ms) but was {}ms",
-            delay.as_millis() * 3,
-            duration.as_millis()
-        );
-        assert!(
-            duration < delay * 4,
-            "Duration should be less than 4 times the delay ({}ms) but was {}ms",
-            delay.as_millis() * 4,
-            duration.as_millis()
-        );
     }
 }