Initial testing utilities #1176

Nemo157 · 2018-08-04T17:10:21Z

I'm not a massive fan of how the generics worked out here, especially around with_context needing two _, _ at the end... any suggestion on a more ergonomic API?

CC #1169

MajorBreakfast · 2018-08-04T18:09:12Z

I'd prefer the name futures-test because the built-in test crate (contains benchmarking stuff) also uses that name.

MajorBreakfast · 2018-08-04T19:26:58Z

futures-test/src/task/context.rs

+use crate::task::{executor, wake};
+use futures_core::{
+    future::Future,
+    task::{local_waker, Context, Executor, Poll, Wake},


For consistency with the rest of our codebase the non-nested style would be better

Context should not be imported directly. Instead it should always be referred to as task::Context. This decision was made in the 0.2 days. I wasn't around back then, but I think it's because the word "context" is so nondescript.

I think in this case it's better to use a bare Context, this is explicitly a helper for managing that exact type. I think I'll change what's used in the docs though.

In terms of non-nested style, what about

use crate::task::{executor, wake}; use std::{marker::Unpin, mem::PinMut, sync::Arc};

I assume that the first is definitely ok, but what about the second? What's the actual limit on nesting that you want to enforce?

I'd prefer to expand the second one as well. I'd like to align our code style with the code style of the standard library and compiler. I browsed the code just now and it seems that they consistently expand such things. Also, our existing code does so as well:

use core::marker::Unpin; use core::mem::PinMut;

can be found many many times in futures-rs 🙂

Alright, so nesting the final segment in a path is ok, but no more than that.

(I assume the main reason that the compiler expands everything is that being able to nest more than a single segment is relatively recent, only since March)

MajorBreakfast · 2018-08-04T19:31:52Z

I think the symlinked license files are missing

MajorBreakfast · 2018-08-05T06:55:59Z

especially around with_context needing two _, _ at the end... any suggestion on a more ergonomic API?

Maybe impl Trait can help

MajorBreakfast · 2018-08-05T07:09:46Z

futures-test/src/task/context.rs

+///
+/// let mut future = PinBox::new(async {
+///     5
+/// });


I suggest

let mut future = async { 5 }; pin_mut!(future);

I think we should prefer the pin_mut! macro in examples because users should reach for it first to avoid the heap allocation

Nemo157 · 2018-08-05T09:50:21Z

Maybe impl Trait can help

Not for this case, since you can't use turbofish when calling the function to fill in the 2 required parameters.

Nemo157 · 2018-08-05T10:06:24Z

😭 features and dev-dependencies seem impossible to work with (I knew there were going to be issues with using futures-test in a no_std crate, but I can't even get it working properly for futures to use it).

Nemo157 · 2018-08-05T12:16:24Z

And that's the entirety of futures/test/support moved over, I think that's probably a good initial cut.

MajorBreakfast · 2018-08-05T14:54:41Z

futures-test/src/assert.rs

+/// #![feature(async_await, futures_api, pin)]
+///
+/// #[macro_use]
+/// extern crate futures_test;


In other doc example we write this as:

#![feature(async_await, futures_api, pin)] #[macro_use] extern crate futures_test;

MajorBreakfast · 2018-08-05T14:56:25Z

futures-test/src/assert.rs

+/// extern crate futures_test;
+///
+/// #[macro_use]
+/// extern crate futures;


I think this isn't required. We rely on the Rust 2018 module system and there's also no macro from the futures crate in use

pin_mut! is coming from futures still, this can change to pin_utils or whatever once that's here.

ah right ^^'

MajorBreakfast · 2018-08-05T14:57:05Z

futures-test/src/assert.rs

+/// #[macro_use]
+/// extern crate futures;
+///
+/// # fn main() {


Main function doesn't need to be explicit

It does when using #[macro_use]

😏
https://github.com/rust-lang-nursery/futures-rs/blob/86c40ff7e0d5c28e37a14edd3732e25efdfb9805/futures-util/src/macros/pin.rs#L88-L94

Ah, but only when #[macro_use] is on the same line as extern crate...

Oh ^^' interesting limitation

futures-test/src/assert.rs

+/// let mut stream = stream::once((async { 5 }).delay());
+/// pin_mut!(stream);
+///
+/// assert_stream_pending!(stream);


MajorBreakfast · 2018-08-05T17:03:22Z

I'm not so sure about the TestContext API:

TestContext is not actually a Context although its name suggests it
Its name will often be shorted to "context" which leads to two things being called "context"
It is not no_std compatible
TestContext::poll works the other way around

Here is a suggestion for an alternative API:

let cx = Context::new(&NoopWaker, &NoopExecutor);
let cx = noop_context(); // Short hand

let cx = Context::new(&NoopWaker, &PanicExecutor);
let cx = no_spawn_context(); // Short hand

let wake_counter = WakeCounter::new();
let cx = noop_context().with_waker(wake_counter.local_waker());
...
wake_counter.count(); // usize

let spawn_recorder = SpawnRecorder::new();
let cx = noop_context().with_executor(spawn_recorder.executor());
...
spawn_recorder.futures(); // Vec<FutureObj<'static, ()>>, later Vec<Box<dyn Future<Output = ()> + Send + 'static>>

Everything except SpawnRecorder is no_std compatible. WakeCounter just needs atomics.

MajorBreakfast · 2018-08-05T17:36:59Z

futures-test/src/lib.rs

+#[macro_use]
+extern crate futures_util;
+
+#[macro_export]


#[macro_export] above modules is not required

Nemo157 · 2018-08-05T17:50:09Z

The closest I can work out how to do is:

let wake = wake::panic();
let mut executor = executor::Panic;
let cx = Context::new(&wake, &mut executor);

the waker is a LocalWaker, which is non-Sync so can't be a static, so must be created from a function call. Then because the context takes in references the waker and executor both have to be stored in local variables to get a non-temporary lifetime. For SpawnRecorder specifically it would also become much more complicated since it would need to start using thread safe internal mutability.

That's the main reason I wanted a type like TestContext to handle the creation and lifetimes for the waker and executor, with the goal of compressing those three lines into one.

I don't think being no_std compatible is very worthwhile (at least I have not seen any way to actually utilise it in test helpers), it's also basically impossible because of limitations in feature handling with dev-dependencies in Cargo.

MajorBreakfast · 2018-08-05T19:05:12Z

I agree that no_std compatibility for testing utilities is not important. Running tests on embedded devices doesn't make much sense. At least not unit tests with dummy wakers and executors.
You're right: The lifetimes of my code above don't work out
I continue to believe that we shouldn't introduce poll and poll_unpin methods on the TestContext. They are unintuitive because they work the wrong way around and they can't be used with streams and sinks.
The code examples should refrain from shortening variables called test_context to context. test_cx is fine though.

And how about introducing:

with_noop_context(|cx| { ... }); // waker and executor are noops
with_no_spawn_context(|cx| { ... }); // waker is a noop, executor panics on spawn attempts
let cx = test_cx.context(); // For use with NLL

Nemo157 · 2018-08-06T07:29:52Z

I continue to believe that we shouldn't introduce poll and poll_unpin methods on the TestContext. They are unintuitive because they work the wrong way around and they can't be used with streams and sinks.

My issue was that using with per call is a lot of semantic overhead that makes the poll hard to read

assert_eq!(Poll::Pending, test_cx.with(|cx| abortable_rx.poll_unpin(cx)));

I think that would be helped a lot with the context method + NLL you suggest, while it's more code it's simpler to read code than what you get with with (although, I'm not sure if NLL adds any extra promotion that would allow this to work without test_cx.context() being saved to a local variable)

assert_eq!(Poll::Pending, abortable_rx.poll_unpin(&mut test_cx.context()));

The code examples should refrain from shortening variables called test_context to context. test_cx is fine though.

👍

with_noop_context(|cx| { ... }); // waker and executor are noops
with_no_spawn_context(|cx| { ... }); // waker is a noop, executor panics on spawn attempts
let cx = test_cx.context(); // For use with NLL

Yep, I realized that a context method should work last night. Didn't have time to fully test it. I'll add these soon.

I'm wondering if there's some lifetime finagling that would make it possible to have

let cx = noop_context();
let cx = panic_context();
let cx = no_spawn_context();

Since those wakers/executors don't actually do anything having multiple mutable references to a static executor should be safe (but I'm going to have to investigate how to make it sound...), LocalWaker on the other hand I'm wondering if there's a way to leverage thread local storage to have static instances.

MajorBreakfast · 2018-08-06T08:36:22Z

@Nemo157 If you could make noop_context() and friends work via thread local storage, that would IMO be truly awesome. It could make this possible:

future.poll(noop_context());

Edit: Depends of course on the return type (if it's a reference or a value) There could also be two functions: One that returns a ref, one that returns a value

MajorBreakfast · 2018-08-08T17:12:09Z

futures-test/src/assert.rs

+        let cx = &mut $crate::task::no_spawn_context();
+        match $crate::futures_core_reexport::stream::Stream::poll_next(
+            stream, cx,
+        ) {


I recommend using

if poll.is_ready() {}

MajorBreakfast · 2018-08-08T17:16:04Z

futures-test/src/task.rs

@@ -1,10 +1,19 @@
 //! Task related utilities.
+//!
+//! For the majority of usecases you can use the functions exported below


In the majority of use cases

and "functions" is mentioned twice in this sentence

Nemo157 · 2018-08-08T17:16:43Z

futures-test/src/task/executor/noop.rs

+///
+/// let mut cx = panic_context();
+/// let mut executor = executor::Noop::new();
+/// let cx = &mut cx.with_executor(&mut executor);


Two issues with Context's current APIs here:

The first (that's repeated in most of the examples) is that with_executor and with_waker take &mut self, that's useful for being able to create a sub-context with a different executor inside a poll function, but it is what makes the first of these three lines necessary. If with_executor instead took self by value we should be able to merge the first and third lines into

let cx = &mut panic_context().with_executor(&mut executor);

Second, with_executor requires E: Sized so we can't use the unsized Noop::executor_mut(), if we removed this bound then we can merge the second and third lines into

let cx = &mut cx.with_executor(executor::Noop::executor_mut());

One way to allow taking the context by value while still being able to create a sub-context inside poll could be to add

impl<'a> Context<'a> { pub fn reborrow<'b>(&'b mut self) -> Context<'b> { Context { local_waker: self.local_waker, executor: self.executor, } }

Then the WithExecutor combinator would just have to change

diff --git futures-util/src/future/with_executor.rs futures-util/src/future/with_executor.rs index 72b7322e8..f283b7b5a 100644 --- futures-util/src/future/with_executor.rs +++ futures-util/src/future/with_executor.rs @@ -32,6 +32,6 @@ impl<Fut, E> Future for WithExecutor<Fut, E> let this = unsafe { PinMut::get_mut_unchecked(self) }; let fut = unsafe { PinMut::new_unchecked(&mut this.future) }; let exec = &mut this.executor; - fut.poll(&mut cx.with_executor(exec)) + fut.poll(&mut cx.reborrow().with_executor(exec)) } }

MajorBreakfast · 2018-08-08T17:18:10Z

futures-test/src/task.rs

+//! to use in your tests.
+//!
+//! For more complex test cases you can take a `Context` from one of these
+//! functions then use the


*and * then

MajorBreakfast · 2018-08-08T17:31:24Z

futures-test/src/assert.rs

+/// ```
+/// #![feature(async_await, futures_api, pin)]
+///
+/// use futures::stream;


Our code examples usually have no free line between #![feature] and use statements

MajorBreakfast · 2018-08-08T17:33:18Z

futures-test/src/task/executor.rs

@@ -0,0 +1,8 @@
+//! Implementations of [`Executor`][futures_core::task::Executor] with various


Maybe rename this to executor/mod.rs instead.

Yeah, I've been getting quite annoyed at the new file scheme breaking tab completion, so will switch this back.

They changed it because the old system can lead to loads of mod.rs file tabs in an editor. However, that's IMO fine if these files are just lightweight files that just reexport the right things from their child modules. So, I continue to like mod.rs files 🙂

MajorBreakfast · 2018-08-08T17:34:34Z

futures-test/src/task/executor.rs

+mod panic;
+mod record;
+
+pub use self::{noop::Noop, panic::Panic, record::Record};


I prefer the

mod noop; use self::noop::Noop; mod panic; use self::panic::Panic; // --snip--

style

MajorBreakfast · 2018-08-17T18:32:56Z

@Nemo157 I changed various things. Can you have a look?

Nemo157 force-pushed the testing-utilities branch from d7a989c to fd4ac19 Compare August 4, 2018 17:26

MajorBreakfast reviewed Aug 4, 2018

View reviewed changes

MajorBreakfast reviewed Aug 5, 2018

View reviewed changes

futures-test/src/assert.rs

/// let mut stream = stream::once((async { 5 }).delay());

/// pin_mut!(stream);

///

/// assert_stream_pending!(stream);

This comment was marked as resolved.

Sign in to view

MajorBreakfast reviewed Aug 5, 2018

View reviewed changes

Nemo157 force-pushed the testing-utilities branch from 2363782 to 5a16b71 Compare August 6, 2018 17:33

MajorBreakfast reviewed Aug 8, 2018

View reviewed changes

Nemo157 commented Aug 8, 2018

View reviewed changes

MajorBreakfast reviewed Aug 8, 2018

View reviewed changes

Nemo157 force-pushed the testing-utilities branch 3 times, most recently from 23dd60b to 0473cda Compare August 15, 2018 10:38

Initial testing utilities crate

8a5f1a2

Nemo157 force-pushed the testing-utilities branch from 0473cda to 8a5f1a2 Compare August 15, 2018 10:39

Nemo157 mentioned this pull request Aug 15, 2018

(Modules) Tracking issue for the mod.rs changes rust-lang/rust#53125

Closed

3 tasks

Various tweaks

82241d5

Merge branch 'master' into testing-utilities

e3db5e6

MajorBreakfast merged commit 8fb41ab into rust-lang:master Aug 17, 2018

Nemo157 deleted the testing-utilities branch February 16, 2019 16:15

		@@ -0,0 +1,8 @@
		//! Implementations of [`Executor`][futures_core::task::Executor] with various

Initial testing utilities #1176

Initial testing utilities #1176

Conversation

Nemo157 commented Aug 4, 2018

MajorBreakfast commented Aug 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nemo157 Aug 5, 2018 • edited Loading

Choose a reason for hiding this comment

MajorBreakfast commented Aug 4, 2018

MajorBreakfast commented Aug 5, 2018

Choose a reason for hiding this comment

Nemo157 commented Aug 5, 2018

Nemo157 commented Aug 5, 2018

Nemo157 commented Aug 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MajorBreakfast Aug 5, 2018 • edited Loading

Choose a reason for hiding this comment

This comment was marked as resolved.

MajorBreakfast commented Aug 5, 2018 • edited Loading

Choose a reason for hiding this comment

Nemo157 commented Aug 5, 2018

MajorBreakfast commented Aug 5, 2018 • edited Loading

Nemo157 commented Aug 6, 2018

MajorBreakfast commented Aug 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MajorBreakfast commented Aug 17, 2018

Nemo157 Aug 5, 2018 •

edited

Loading

MajorBreakfast Aug 5, 2018 •

edited

Loading

MajorBreakfast commented Aug 5, 2018 •

edited

Loading

MajorBreakfast commented Aug 5, 2018 •

edited

Loading

MajorBreakfast commented Aug 6, 2018 •

edited

Loading