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

Refactor test_runner.rs as a folder #46

Merged
merged 2 commits into from
Mar 11, 2018

Conversation

Centril
Copy link
Collaborator

@Centril Centril commented Mar 11, 2018

This PR is a pure refactoring and should have zero semantic or public changes.


I was originally planning on changing the signature of run from:

pub fn run<S, F>(&mut self, strategy: &S, test: F)
    -> Result<(), TestError<ValueFor<S>>>
where
    S : Strategy,
    F : Fn (&ValueFor<S>) -> TestCaseResult

to:

pub fn run<S, F>(&mut self, strategy: &S, test: F)
    -> Result<(), TestError<S::Value>> // <-- Note that the ValueTree is returned
where
    S : Strategy,
    F : Fn(ValueFor<S>) -> TestCaseResult // <-- Note the absence of & here.

but right now it didn't seem worth it to make that change. However - I think we should do that change once GATs land (and try it out in nightly) since it should allow us to address #9 as well as giving users an owned value in their tests without having to clone test case values (and not all types implement clone..).

@AltSysrq
Copy link
Collaborator

Thanks for this, it's increasingly felt necessary but I never got around to it.

FTR, your PR description contains the text fix #9, which github interprets to mean that merging this PR will close that issue; I'm going to take the liberty to edit that to something else to avoid close/reopen noise.

@AltSysrq AltSysrq merged commit 480674b into proptest-rs:master Mar 11, 2018
@Centril
Copy link
Collaborator Author

Centril commented Mar 11, 2018

Oh thanks, I forgot about that =)

@Centril Centril deleted the feature/runner-refactor branch March 13, 2018 03:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants