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: Make Mock both Send and Sync #3594

Merged
merged 7 commits into from
Mar 10, 2021
Merged

Conversation

hakejam
Copy link
Contributor

@hakejam hakejam commented Mar 9, 2021

Previously, the inner 'rx' of the Mock IO object used an unbound
receiver channel for mock 'Actions'. This allowed Mock to auto
implement Sync. Mock was recently converted to use dynamic
dispatch, which now needs the marker Sync to continue to
implement these traits.

Motivation

I am upgrading our package to use Tokio 1.0. This updated tokio-test from 0.2.1 to 0.4. In the current version, Mock is not Sync.

Solution

This change adds the marker trait Sync to the dynamic dispatch, which should make Mock both Send + Sync.

Previously, the inner 'rx' of the Mock IO object used an unbound
receiver channel for mock 'Actions'. This allowed the Send auto
trait implentation to match this object. Mock was recently
convernted to use dynamic dispatch, which now needs the marker
Send internally inorder for it to also be Send.
@hakejam
Copy link
Contributor Author

hakejam commented Mar 9, 2021

@LucioFranco I don't normally do PRs, but when I do, its for Tokio :)

tokio-test/src/io.rs Outdated Show resolved Hide resolved
@Darksonn Darksonn added the A-tokio-test Area: The tokio-test crate label Mar 9, 2021
Jake Ham added 2 commits March 9, 2021 09:36
Updated the mock object to use the `UnboundReceiverStream` from
tokio-stream.
Update the Tokio dependency to use the sub-crate vended through
the path.
Copy link
Contributor

@Darksonn Darksonn left a comment

Choose a reason for hiding this comment

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

Does it work if you remove the path dep from tokio-test instead of adding it to tokio-stream?

Jake Ham added 2 commits March 9, 2021 09:49
Removed the path based sub-crate for Tokio.
Updated the Tokio dependency to not use the path based sub-crate
syntax.
Copy link
Contributor

@Darksonn Darksonn left a comment

Choose a reason for hiding this comment

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

Once you get the path dep stuff to compile, this looks good to me. If you can't get it to work when removing the path deps, feel free to add them back.

@LucioFranco
Copy link
Member

@hakejam thank for doing this!

I don't quite understand this test failure? Does this code need to be rebased? @Darksonn

@Darksonn
Copy link
Contributor

I'm not entirely sure either. I don't think rebasing will help.

@hakejam
Copy link
Contributor Author

hakejam commented Mar 10, 2021

I spent some time yesterday trying to remove the paths, but couldn't get it working. The main tokio crate tests are failing because tokio-test Mock doesn't implement the same trait AsyncReadExt. I might have more time today to look into this if you need me to.

@Darksonn
Copy link
Contributor

I'm going to merge this PR as-is once CI passes, although I would like to know what is up and fix it in a later PR if possible.

@LucioFranco LucioFranco merged commit 6919f7c into tokio-rs:master Mar 10, 2021
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants