From 71deaa3c7c029644b76a2d4b462b558a7faa9340 Mon Sep 17 00:00:00 2001
From: Nick Lanham <nicklan@users.noreply.github.com>
Date: Fri, 13 Dec 2024 13:28:45 -0800
Subject: [PATCH] don't use std abs_diff, put it in test_utils instead, run
 tests with msrv in action (#596)

<!--
Thanks for sending a pull request!  Here are some tips for you:
1. If this is your first time, please read our contributor guidelines:
https://github.com/delta-incubator/delta-kernel-rs/blob/main/CONTRIBUTING.md
2. Run `cargo t --all-features --all-targets` to get started testing,
and run `cargo fmt`.
  3. Ensure you have added or run the appropriate tests for your PR.
4. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP]
Your PR title ...'.
  5. Be sure to keep the PR description updated to reflect all changes.
-->

## What changes are proposed in this pull request?
<!--
Please clarify what changes you are proposing and why the changes are
needed.
The purpose of this section is to outline the changes, why they are
needed, and how this PR fixes the issue.
If the reason for the change is already explained clearly in an issue,
then it does not need to be restated here.
1. If you propose a new API or feature, clarify the use case for a new
API or feature.
  2. If you fix a bug, you can clarify why it is a bug.
-->

- create an abs_diff function in test_utils (same code as in std)
- use that in the two places we wanted it instead
- also add a workflow to make sure that we can always compile against
msrv


<!--
Uncomment this section if there are any changes affecting public APIs:
### This PR affects the following public APIs

If there are breaking changes, please ensure the `breaking-changes`
label gets added by CI, and describe why the changes are needed.

Note that _new_ public APIs are not considered breaking.
-->


## How was this change tested?
<!--
Please make sure to add test cases that check the changes thoroughly
including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please
clarify how you tested, ideally via a reproducible test documented in
the PR description.
-->

- Existing tests pass
- New workflow passes
---
 .github/workflows/build.yml             | 28 +++++++++++++++++++++++++
 kernel/src/engine/default/filesystem.rs |  4 ++--
 kernel/src/engine/sync/fs_client.rs     |  4 +++-
 test-utils/src/lib.rs                   | 10 +++++++++
 4 files changed, 43 insertions(+), 3 deletions(-)

diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml
index 746a05e37..270dd3c45 100644
--- a/.github/workflows/build.yml
+++ b/.github/workflows/build.yml
@@ -41,6 +41,34 @@ jobs:
           cargo msrv --path derive-macros/ verify --all-features
           cargo msrv --path ffi/ verify --all-features
           cargo msrv --path ffi-proc-macros/ verify --all-features
+  msrv-run-tests:
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v4
+      - name: Install minimal stable and cargo msrv
+        uses: actions-rs/toolchain@v1
+        with:
+          profile: default
+          toolchain: stable
+          override: true
+      - uses: Swatinem/rust-cache@v2
+      - name: Install cargo-msrv
+        shell: bash
+        run: |
+          cargo install cargo-msrv --locked
+      - name: Get rust-version from Cargo.toml
+        id: rust-version
+        run: echo "RUST_VERSION=$(cargo msrv show --path kernel/ --output-format minimal)" >> $GITHUB_ENV
+      - name: Install specified rust version
+        uses: actions-rs/toolchain@v1
+        with:
+          toolchain: ${{ env.RUST_VERSION }}
+          profile: minimal
+      - name: run tests
+        run: |
+          pushd kernel
+          echo "Testing with $(cargo msrv show --output-format minimal)"
+          cargo +$(cargo msrv show --output-format minimal) test
   docs:
     runs-on: ubuntu-latest
     env:
diff --git a/kernel/src/engine/default/filesystem.rs b/kernel/src/engine/default/filesystem.rs
index 36b110774..5606a28d0 100644
--- a/kernel/src/engine/default/filesystem.rs
+++ b/kernel/src/engine/default/filesystem.rs
@@ -160,7 +160,7 @@ mod tests {
     use object_store::memory::InMemory;
     use object_store::{local::LocalFileSystem, ObjectStore};
 
-    use test_utils::delta_path_for_version;
+    use test_utils::{abs_diff, delta_path_for_version};
 
     use crate::engine::default::executor::tokio::TokioBackgroundExecutor;
     use crate::engine::default::DefaultEngine;
@@ -241,7 +241,7 @@ mod tests {
         assert!(!files.is_empty());
         for meta in files.into_iter() {
             let meta_time = Duration::from_millis(meta.last_modified.try_into().unwrap());
-            assert!(meta_time.abs_diff(begin_time) < Duration::from_secs(10));
+            assert!(abs_diff(meta_time, begin_time) < Duration::from_secs(10));
         }
     }
     #[tokio::test]
diff --git a/kernel/src/engine/sync/fs_client.rs b/kernel/src/engine/sync/fs_client.rs
index a5c889da5..9577b1499 100644
--- a/kernel/src/engine/sync/fs_client.rs
+++ b/kernel/src/engine/sync/fs_client.rs
@@ -84,6 +84,8 @@ mod tests {
     use itertools::Itertools;
     use url::Url;
 
+    use test_utils::abs_diff;
+
     use super::SyncFilesystemClient;
     use crate::FileSystemClient;
 
@@ -111,7 +113,7 @@ mod tests {
         assert!(!files.is_empty());
         for meta in files.iter() {
             let meta_time = Duration::from_millis(meta.last_modified.try_into()?);
-            assert!(meta_time.abs_diff(begin_time) < Duration::from_secs(10));
+            assert!(abs_diff(meta_time, begin_time) < Duration::from_secs(10));
         }
         Ok(())
     }
diff --git a/test-utils/src/lib.rs b/test-utils/src/lib.rs
index e43e8481c..2605bea56 100644
--- a/test-utils/src/lib.rs
+++ b/test-utils/src/lib.rs
@@ -106,3 +106,13 @@ pub fn into_record_batch(engine_data: Box<dyn EngineData>) -> RecordBatch {
         .unwrap()
         .into()
 }
+
+/// We implement abs_diff here so we don't have to bump our msrv.
+/// TODO: Remove and use std version when msrv >= 1.81.0
+pub fn abs_diff(self_dur: std::time::Duration, other: std::time::Duration) -> std::time::Duration {
+    if let Some(res) = self_dur.checked_sub(other) {
+        res
+    } else {
+        other.checked_sub(self_dur).unwrap()
+    }
+}