Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

don't use std abs_diff, put it in test_utils instead, run tests with msrv in action #596

Merged
merged 2 commits into from
Dec 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions .github/workflows/build.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
4 changes: 2 additions & 2 deletions kernel/src/engine/default/filesystem.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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]
Expand Down
4 changes: 3 additions & 1 deletion kernel/src/engine/sync/fs_client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,8 @@ mod tests {
use itertools::Itertools;
use url::Url;

use test_utils::abs_diff;

use super::SyncFilesystemClient;
use crate::FileSystemClient;

Expand Down Expand Up @@ -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(())
}
Expand Down
10 changes: 10 additions & 0 deletions test-utils/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have a tracking issue for this so we don't forget?
(really it would be neat if rust had a way to annotate these sorts of workarounds, so they start failing once MSRV bumps)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good call :)

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()
}
}
Loading