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.

Sign up for GitHub

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

Jump to bottom

Enable using ? in tests #25

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Enable using ? in tests #25

wants to merge 3 commits into from

Conversation

bittrance
Copy link

When writing tests for code that does I/O or which requires setup that can theoretically fail, you may end up with a lot of expect/unwrap boiler plate. Unit tests can return Result<(), E> which enables using the ? operator to return early on error cases, but there is no way to achieve this in speculate.rs.

This PR allows describe blocks to specify that their tests can return Result<(), E> by including an errtype(E) entry which will make all the tests in that describe block and any nested describe blocks return Result<(), E>. This includes any before or after blocks. For example:

speculate! {
  errtype(Error)

  before {
    fs::write("/the/file", "")?;
  }

  it "reads the file" {
    let data = read_the_file()?;
    assert!(data.len() > 0);
  }
}

This will desugar into a test which returns Result<(), Error> and gets an Ok(()) appended at the end, e.g.

#[test]
fn test_reads_the_file() -> Result<(), Error> {
  fs::write("/the/file", "")?;
  let data = read_the_file()?;
  assert!(data.len() > 0);
  Ok(())
}

I think using a predicate like this makes sense because it should not cause trouble, even if some or all of the covered tests happen not to contain ?. Thus, this PR contains no way to annotate a specific test or for a nested block to opt out.

There is one special case, namely that Rust requires #[should_panic] tests to return (). Given that such tests are typically sprinkled among other tests, it would be annoying to have to isolate them in their own describe. Instead, this PR detects such tests and avoids turning them into -> Result<(), E>.

Replaces #24.

bittrance and others added 2 commits April 17, 2019 16:24
@bittrance
Describe blocks can now specify that their tests return Result<(), E>…
c108ae1 
… using `errtype(E)`.
errtype respects should_panics.
d03c815 
Rust does not allow returning Result from tests that are expected to panic.
@bittrance bittrance mentioned this pull request Apr 17, 2019
Closed
@utkarshkukreti
Copy link
Owner

Instead of a special errtype statement, what do you think about adding a return type annotation similar to what #[test] requires:

it "has i/o setup" -> Result<(), std::io::Error> {
    fs::write("/some/file", "with content")?;
    assert_eq!(file_reader()?, "with_content");
    Ok(())
}

I don't think we should add a special kind of statement for this.

@bittrance
Copy link
Author

bittrance commented Apr 24, 2019 •
edited
Loading

I care about 1) not having to write a lot of essentially meaningless Ok(()) and 2) not having to repeat the error/result type for every single test. I'm perfectly fine with declaring the entire Result type rather than just the error type so as to make it slightly less cryptic. I also think it should be possible to annotate a single test.

My understanding is that adding an implicit Ok(()) is unlikely to cause anyone issues even if they do not know that it happens.

I think it is likely that either none or most tests in a speculate!/describe block will want to "get rid of" errors. Thus it makes sense to mark a whole block as "resulty". I would have preferred putting something in the actual describe entry (e.g. describe -> Result<(), Error> {...}) but as far as I can tell, that is not possible on the speculate! {...} level and therefore I opted for a statement so as not to force nesting of "resulty" tests in an additional describe. Putting the entry at describe level has the additional benefit of making it intuitive that before and after blocks with ? in them are "covered" by the auto-Result behavior; putting the error/result declaration on the test means you have to know that the before block is injected into the actual test.

I would be fine with the following styles as well:

describe "stuff" -> Result<(), Error> {...}
describe "stuff" default Result<(), Error> {...}
describe "stuff" returns Result<(), Error> {...}
describe "stuff" { test_returns(Result<(), Error>) ... }

Anyone that catches your fancy?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@bittrance @utkarshkukreti