-
Notifications
You must be signed in to change notification settings - Fork 68
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
Conversation
2dcf9e8
to
d788602
Compare
@twuebi Please fix conflicts |
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 |
There was a problem hiding this 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!
There was a problem hiding this 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.
}
There was a problem hiding this 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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
#[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.