Skip to content

feat: add a test macro #170

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

Merged
merged 8 commits into from
Jun 23, 2025
Merged

feat: add a test macro #170

merged 8 commits into from
Jun 23, 2025

Conversation

twuebi
Copy link
Contributor

@twuebi twuebi commented Jun 18, 2025

  1. Added a test macro #[minio_macros::test], it performs setup and tear down for tests (currently creation + deletion of bucket), if no bucket is wished #[minio_macros::test(no_bucket)] will be your friend. If the test panics, it'll catch the unwind and attempt cleanup before resuming the unwind.
  2. replace some blocking io in tests with async_std variants

@twuebi twuebi added enhancement Used in release doc generation labels Jun 18, 2025
@twuebi twuebi force-pushed the tp/test-macro branch 2 times, most recently from 2dcf9e8 to d788602 Compare June 18, 2025 16:02
@donatello
Copy link
Member

@twuebi Please fix conflicts

@twuebi
Copy link
Contributor Author

twuebi commented Jun 18, 2025

I'm still tracking down some functions that seem to take too long between await points leading to some really slow/hanging tests when running on the standard tokio runtime (https://ryhl.io/blog/async-what-is-blocking/). I think our impl Stream for RandSrc may be the culprit, calls to poll_next take anything between 200 - 800 microseconds, will have to look a bit more into it.

Copilot

This comment was marked as outdated.

Copy link
Member

@HJLebbink HJLebbink left a comment

Choose a reason for hiding this comment

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

LGTM, thanx for cleaning up!

@HJLebbink HJLebbink requested a review from Copilot June 20, 2025 11:19
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the test suite and some client utility functions to adopt a new asynchronous testing approach by replacing the traditional #[tokio::test] attribute with a custom #[minio_macros::test] macro and switching blocking IO operations to async_std variants.

  • Replaces tokio test attributes with the new custom test macro that supports bucket setup/cleanup and express mode skipping.
  • Converts blocking file operations to async_std counterparts and updates related functions to use Arc for improved async behavior.
  • Updates environment variable references and adjusts benchmarks and cleanup logic accordingly.

Reviewed Changes

Copilot reviewed 42 out of 42 changed files in this pull request and generated no comments.

Show a summary per file
File Description
tests/*.rs Refactored tests to use async_std and the new test macro; adjusted test signatures and environment variable usage.
src/s3/utils.rs, src/s3/types.rs Updated APIs to use Arc for SegmentedBytes and improved async compatibility.
src/s3/client.rs Adapted client methods to spawn blocking tasks for SHA256 hashing and use Arc for body handling.
macros/ Introduced the custom test attribute macro enhancing test setup and cleanup.
common/ Updated environment variable references and asynchronous bucket cleanup.
benches/ Adjusted environment variable usage and minor code style improvements.
Comments suppressed due to low confidence (2)

common/src/rand_src.rs:49

  • The read chunk size has been reduced from 64 KiB to 8 KiB. Confirm that this smaller chunk size is intentional as it may affect performance when reading large amounts of data.
        let bytes_read = self.size.min(8 * 1024) as usize;

src/s3/client.rs:452

  • [nitpick] Using async_std::task::spawn_blocking for SHA256 hashing offloads the computation but may introduce overhead when invoked frequently. Review the performance impact under high load to ensure it meets your throughput requirements.
                        }

Copy link
Member

@donatello donatello left a comment

Choose a reason for hiding this comment

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

LGTM - thanks for adding the current_thread/multi_thread tests!

@HJLebbink HJLebbink self-requested a review June 23, 2025 11:36
Copy link
Member

@HJLebbink HJLebbink left a comment

Choose a reason for hiding this comment

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

LGTM

@twuebi twuebi merged commit 8497fdb into minio:master Jun 23, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Used in release doc generation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants