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

test: Improve tokio_test::task docs #5132

Merged
merged 6 commits into from
Oct 28, 2022
Merged

test: Improve tokio_test::task docs #5132

merged 6 commits into from
Oct 28, 2022

Conversation

LucioFranco
Copy link
Member

cc @rcoh

@LucioFranco LucioFranco requested a review from a team October 27, 2022 14:42
Copy link
Contributor

@rcoh rcoh left a comment

Choose a reason for hiding this comment

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

LGTM, but I'd add an example in both spots so that the function is obvious how to use

@@ -11,7 +36,7 @@ use std::task::{Context, Poll, RawWaker, RawWakerVTable, Waker};

use tokio_stream::Stream;

/// TODO: dox
/// Spawn a future into a [`Spawn`] which wraps the future in a mocked executor.
Copy link
Contributor

Choose a reason for hiding this comment

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

recommend also adding an example here

Copy link
Contributor

Choose a reason for hiding this comment

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

I would add more "so what" / why docs here. maybe something like:

As a side note, I'd probably make another function for streams and bound the generic—I get that they are functionally the same but it's not at all obvious you can pass a stream in here & calling spawn on a stream doesn't really make sense.

/// Test utility to spawn the [`Future`] or [`Stream`] into a [`Spawn`] container
///
/// The `Spawn` container enables direct polling and introspection of the state of the future which is often required
/// when testing code that implements [`Future`] or [`Stream`]. The [`Spawn`] container enables you to poll the 
/// future directly without requiring you to construct your own `Context` or `Waker`. It also handles the bookkeeping required to track wakeups & future state.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was keeping the example at the mod docs so ill just point to that.

Copy link
Contributor

@rcoh rcoh left a comment

Choose a reason for hiding this comment

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

LGTM, but I'd consider fleshing these docs out a bit more. This is an extremely helpful feature and the more key words you can add for SEO the better, eg "test context" "test waker", "test custom future", call poll without context

@@ -11,7 +36,7 @@ use std::task::{Context, Poll, RawWaker, RawWakerVTable, Waker};

use tokio_stream::Stream;

/// TODO: dox
/// Spawn a future into a [`Spawn`] which wraps the future in a mocked executor.
Copy link
Contributor

Choose a reason for hiding this comment

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

I would add more "so what" / why docs here. maybe something like:

As a side note, I'd probably make another function for streams and bound the generic—I get that they are functionally the same but it's not at all obvious you can pass a stream in here & calling spawn on a stream doesn't really make sense.

/// Test utility to spawn the [`Future`] or [`Stream`] into a [`Spawn`] container
///
/// The `Spawn` container enables direct polling and introspection of the state of the future which is often required
/// when testing code that implements [`Future`] or [`Stream`]. The [`Spawn`] container enables you to poll the 
/// future directly without requiring you to construct your own `Context` or `Waker`. It also handles the bookkeeping required to track wakeups & future state.

///
/// This can be used to spawn a [`Future`] or a [`Stream`].
///
/// For more information, check the module docs.
Copy link
Contributor

Choose a reason for hiding this comment

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

link?

Comment on lines +51 to +52
/// Future spawned on a mock task that can be used to poll the future or stream
/// without needing pinning or context types.
Copy link
Contributor

Choose a reason for hiding this comment

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

link to module docs

Comment on lines +121 to +122
/// If `T` is a [`Future`] then poll it. This will handle pinning and the context
/// type for the future.
Copy link
Contributor

Choose a reason for hiding this comment

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

a little misleading—kinda makes me think that the code would be

if t.is_future() { poll } else { do nothing }

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah though not sure how to describe that its a type system if statement rather than a rust language one...

@LucioFranco LucioFranco merged commit b749013 into master Oct 28, 2022
@LucioFranco LucioFranco deleted the lucio/test-docs branch October 28, 2022 17:55
@Darksonn Darksonn added T-docs Topic: documentation A-tokio-test Area: The tokio-test crate labels Oct 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio-test Area: The tokio-test crate T-docs Topic: documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants