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

eRFC: Custom test frameworks #2318

Merged
merged 22 commits into from Apr 28, 2018

Conversation

Projects
None yet
@Manishearth
Member

Manishearth commented Feb 1, 2018

@Manishearth

This comment has been minimized.

Member

Manishearth commented Feb 1, 2018

Had a talk with @eddyb who was concerned with compiler impact, and suggested a cleaner form of the alternate proposal that minimizes the impact on the compiler to almost nothing.

Basically, the harness becomes a completely normal proc macro attribute crate, and everything else is provided by the utility crate. Cargo continues to orchestrate it all.

Added it to the alternates section. Thoughts?

https://github.com/Manishearth/rfcs/blob/post-build-contexts/text/0000-erfc-post-build-contexts.md#alternative-procedural-macro-with-minimal-compiler-changes

@Centril

This comment has been minimized.

Contributor

Centril commented Feb 1, 2018

@Manishearth How does that fit with default folders and default sets?

That section feels a bit underspecified atm - not sure I fully understand what it entails.

@Manishearth

This comment has been minimized.

Member

Manishearth commented Feb 1, 2018

Currently default folders and sets are left unresolved, and the idea was that if we wanted to implement those they would be done in the context's Cargo.toml. I'll expand that into a section of the RFC

@Centril

This comment has been minimized.

Contributor

Centril commented Feb 1, 2018

For future reference, the original idea I had with default folders & sets were that you specify them as:

#[post_build_context(
    test, attributes(foo, bar),
    default_folders("src", "test"),
    default_sets("test"))]
pub fn like_todays_test(items: &[AnnotatedItem]) -> TokenStream {
    // ...
}

But doing it via Cargo.toml should be workable as well.

@Manishearth

This comment has been minimized.

Member

Manishearth commented Feb 1, 2018

To avoid confusion, I uplifted all the ideas from "other questions" and put them under "unresolved questions" as proper sections, some of which have partial solutions.

@jonhoo

This comment has been minimized.

Contributor

jonhoo commented Feb 1, 2018

This seems to have missed a bunch of changes made in jonhoo@3f4425e — was that intentional?

@Manishearth

This comment has been minimized.

Member

Manishearth commented Feb 1, 2018

I wasn't aware of those patches. If you had pushed them to my branch, sorry, I'd forgotten that I'd asked you to do that :)

It seems like the main missing thing is renaming it from post-build to build-context? I find that build-context would be confusing with custom profiles (which is the feature that comes to mind when you say "build profile"). We should add it in the list of alternative names. I don't think we should bikeshed this much now; seems like a question for the final non-experimental RFC.

All the other changes seem to be addressed in some form here. If not, let me know.

@Manishearth

This comment has been minimized.

Member

Manishearth commented Feb 1, 2018

(I'm aware I said "this is better" for the term "build contexts", but that was relative to "execution contexts". I already mentioned in the thread why I didn't like "build contexts")

@jonhoo

This comment has been minimized.

Contributor

jonhoo commented Feb 1, 2018

Let me factor out the parts of that change related to the post-build context renaming. Many of the changes are about clarifying the proposed syntax and the examples, but others are somewhat larger. For example, some of the things we talked about regarding folders, single-target, etc. I'll link here once I've done it.

@Manishearth

This comment has been minimized.

Member

Manishearth commented Feb 1, 2018

I fixed folders and single-target. I kept the folders key named "folder" because that's the common case but it takes with a string and array value. There's also single-target somewhere here under a similar name.

Clarifying syntax would be nice.

@jonhoo

This comment has been minimized.

Contributor

jonhoo commented Feb 1, 2018

Extracted the changes and rebased in jonhoo@68695a6. I think most of them are things we've talked about in the past. I think folders is better to indicate it can take multiple, but meh. I think we should just specify that single-target is on the producer, instead of putting it on the consumer and say at the end that it should probably be on the consumer.

@jonhoo

This comment has been minimized.

Contributor

jonhoo commented Feb 1, 2018

I still find the description "post-build" wildly confusing, but maybe that's just me. I agree that bikeshedding on the name here is not a good use of time.

@Manishearth

This comment has been minimized.

Member

Manishearth commented Feb 1, 2018

Merged in and pushed, thanks (plus some fixups). Yeah, I'm not very strongly on either side of "folder" vs "folders" so that works. Moving single-target to the producer is great.

I'm not really fond of either cargo context or cargo post-build (I'm now beginning to think that tying it to test might be the best way out, even if many of these things are not tests). Added that to the CLI section of the unresolved questions.

@jonhoo

This comment has been minimized.

Contributor

jonhoo commented Feb 1, 2018

I think we should be wary of referring to this as being about tests, because it causes us to use a pretty limited mental model of going on. To me, what all of this is proposing is a way for a crate to opt in to a build process defined by another crate. But it also isn't quite a full-blown build process, because it's only "transforming some special items" and "produce a binary this way". This is why "execution context" seemed right for me, and cargo context ("run in this context").

Perhaps a better description is "execution harness"? And then cargo harness.

@Manishearth

This comment has been minimized.

Member

Manishearth commented Feb 1, 2018

Yeah. I like having the term "harness" in there.

I still am wary of the "produce a binary this way" thing because from a user's perspective that's an implementation detail (and the binary targets are different so it doesn't map cleanly)

@jonhoo

This comment has been minimized.

Contributor

jonhoo commented Feb 1, 2018

To me "produce a binary" == "produce a main()", even though I know technically that doesn't hold. Using "execution" instead of "binary" sort of sidesteps that issue though.

EDIT: or maybe a better way to say it is "tests are just a different way of executing your code".

@aturon aturon added the T-cargo label Feb 1, 2018

@aturon

This comment has been minimized.

Member

aturon commented Feb 1, 2018

@nrc nrc referenced this pull request Feb 1, 2018

Closed

Benchmarking / cargo bench #2287

@Manishearth Manishearth referenced this pull request Feb 2, 2018

Closed

Tools news #35

@nrc

This comment has been minimized.

Member

nrc commented Feb 4, 2018

I think we should be wary of referring to this as being about tests, because it causes us to use a pretty limited mental model of going on.

I'm pretty wary in the opposite direction! I think there is a clear use case for custom test frameworks and test-like things (benching, fuzzing, etc.). I haven't seen good motivation for a more general feature. I fear adding a feature that is over-abstract and harder to use than necessary for the sake of hypothetical use cases (architecture astronaut-ing at the language level, if you like).

To be concrete, I think anything user-facing should be phrased in terms of testing, rather than 'post-build context' or whatever. I think the occasional (and minor) mismatch between say testing and profiling is easier for users to handle than getting the common case (testing) wrong.

@Manishearth

This comment has been minimized.

Member

Manishearth commented Feb 4, 2018

@nrc You misunderstand us, you're counting "bench" and "fuzz" as "testing-like", we're not. We're not asking to make it more general than "bench, fuzz, etc" but we think that it may be too specific a term to make sense like this. "post-build" was an attempt to capture the category of "test-like" things better.

That said I agree with your point about astronauting. My original proposal had test in it, I can formulate a commit bringing it back to that state if you'd like.

@nrc

I really like where this proposal has ended up! Thanks for all the work you all have put into it. I've left a lot of comments inline, but I think they are all pretty minor. The main theme for me is thinking about how to make this proposal as simple and minimal as possible, and keeping the naming and documentation focussed on tests and test-like tasks, rather than an over-generalisation.

(As an eRFC I'm merging the "guide-level/reference-level" split for now; when we have more concrete
ideas we can figure out how to frame it and then the split will make more sense)

This comment has been minimized.

@nrc

nrc Feb 4, 2018

Member

It would be useful for the reader to provide a summary of the details before diving right in.

## Bencher
Should we be shipping a bencher by default at all (i.e., in libtest)? Could we instead default
`cargo bench` to a `rust-lang-nursery` `bench` crate?

This comment has been minimized.

@nrc

nrc Feb 4, 2018

Member

It's probably worth leaving this question till much later. My hope is that we can have some middle ground of being part of the distribution, but not shipped by default (like RLS, Rustfmt, etc) and bench and a test 2.0 framework can be part of that set.

use proc_macro::TokenStream;
// attributes() is optional
#[post_build_context(attributes(foo, bar))]

This comment has been minimized.

@nrc

nrc Feb 4, 2018

Member

I would prefer using test_framework or test::framework rather than post_build_context

items, not modify them; modification would have to happen with regular
procedural macros. The returned `TokenStream` must declare the `main()`
that is to become the entry-point for the binary produced when this
post-build context is used.

This comment has been minimized.

@nrc

nrc Feb 4, 2018

Member

Could you give an example of the output please?

This comment has been minimized.

@Manishearth

Manishearth Feb 4, 2018

Member

We mention later down that this eRFC doesn't stabilize the output format. I can mention that the output will probably be similar to cargo test

This comment has been minimized.

@Manishearth

Manishearth Feb 4, 2018

Member

Oh, you mean proc macro output.

This comment has been minimized.

@nrc

nrc Feb 4, 2018

Member

Yeah, what the 'main' would look like

## Procedural macro for a new post-build context
A custom post-build context is essentially a whole-crate procedural

This comment has been minimized.

@nrc

nrc Feb 4, 2018

Member

Given that we don't go TokenStream->TokenStream and we don't replace the input, I think we should not describe this as a proc macro. Rather this is like custom derive in that it uses some of the same implementation of proc macros, while being a separate feature (and similarly, I believe we don't need to worry about hygiene, so we can stabilise without going through the details of that for proc macros).

This comment has been minimized.

@Manishearth

Manishearth Feb 4, 2018

Member

Custom derive already falls under the "procedural macro" umbrella; we specify proc-macro=true in custom derive crates.

I think it's fine to use procedural macro as an umbrella term for this kind of stuff.

attributes. How do we deal with collisions (e.g., most test crates will
want the attribute `#[test]`). Do we namespace the attributes by the
context name (e.g., `#[mytest::test]`)? Do we require them to be behind
`#[cfg(mytest)]`?

This comment has been minimized.

@nrc

nrc Feb 4, 2018

Member

Note that namespaced attributes are a thing which is happening, so using that mechanism might be good, or might be confusing with other uses. Requiring disambiguation using cfg seems fine to me and doesn't add any complexity. (we could also fall back to passing the items to all frameworks that have registered an interest, requiring disambiguation only if that causes problems).

The general syntax and toml stuff should be approximately settled on before this eRFC merges, but
iterated on later. Naming the feature is hard, some candidates are:
- test framework

This comment has been minimized.

@nrc

nrc Feb 4, 2018

Member

I vastly prefer this option

dev-dependencies will be semver-merged with the post-build context's
`context-dependencies`. However, this may not be strictly necessary.
Custom derives have a similar problem and they solve it by just asking
users to import the correct crate.

This comment has been minimized.

@nrc

nrc Feb 4, 2018

Member

I feel like following custom derives is the best thing to do here. Long term, we'll want some more general mechanism for proc macros and I assume we could back that backwards compatible for custom derives and test frameworks.

## Default folders and sets
Should a post-build context be able to declare "defaults" for what folders and post-build sets it
should be added to? This might save users from some boilerplate in a large number of situations.

This comment has been minimized.

@nrc

nrc Feb 4, 2018

Member

Yes!

in [this amendment]).
[RFC 2287]: https://github.com/rust-lang/rfcs/pull/2287
[this amendment]: https://github.com/Manishearth/rfcs/pull/1

This comment has been minimized.

@nrc

nrc Feb 4, 2018

Member

We should think about how to handle cross-compilation. It seems to me that a PBC wants to run in the host environment, rather than the target. But that the generated code wants to run in the target environment. What's not clear is if this needs to be customisable. E.g., imagine a test runner which runs on the host, running tests on the target.

The other big thing missing, I think, is error handling - how does a PBC report errors? custom derives just panic, and it might be fine to use that mechanism, however, it is sub-optimal and I hope proc macros will do better. It would be nice if we are future-proof in that respect (i.e., when we get a proc macro error handling mechansim , PBCs can take advantage of it). I think one way of doing so is to take an argument which is a context and error handling methods could be added to the context type in the future. I'm not sure if there are error handling issues which are specific to testing which we might want to think about. (To be clear I'm thinking of errors at compile time, not errors when the tests are run which I assume will need to be covered as part of the output handling stuff).

This comment has been minimized.

@Manishearth

Manishearth Feb 4, 2018

Member

I think "just panic" is fine.

A context argument would be useful anyway. We can move everything to the context argument.

IMO proc macros should also get a context argument, really.

@rfcbot

This comment has been minimized.

rfcbot commented Apr 18, 2018

🔔 This is now entering its final comment period, as per the review above. 🔔

@Centril

This comment has been minimized.

Contributor

Centril commented Apr 18, 2018

(@nrc I don't think a majority is sufficient, see https://internals.rust-lang.org/t/psa-tweaks-to-fcp-process/6775)

@CAD97

This comment has been minimized.

CAD97 commented Apr 25, 2018

Just to get one more voice on it: don't leave out documentation tests! (Or at least put some effort into bringing them towards being first-class tests.)

One of the trickiest things about measuring coverage or mutagen or whatever testing framework is that often times in libraries, documentation tests can be a major part of test coverage. I, at least, primarily write tests as compiled documentation; this is how a function is used. So it's useful to have that in the documentation, whether it's public documentation or internal.

Just to spitball an idea to add to the table: documentation tests are a specialization of integration tests, right? And if I recall correctly from my digging around trying to find binaries to tool for coverage, they work by building integration tests in temporary directories. Maybe rustdoc-test could be a pass that extracts doctests and puts them in e.g. $CARGO_MANIFEST_DIR/target/doctests to be compiled and ran by the test runner as regular integration tests.

@rfcbot

This comment has been minimized.

rfcbot commented Apr 28, 2018

The final comment period is now complete.

@Centril Centril referenced this pull request Apr 28, 2018

Open

Tracking issue for eRFC 2318, Custom test frameworks #50297

0 of 10 tasks complete

@Centril Centril merged commit c25cfbf into rust-lang:master Apr 28, 2018

@Centril

This comment has been minimized.

Contributor

Centril commented Apr 28, 2018

Huzzah! This experimental RFC is now merged!

Tracking issue: rust-lang/rust#50297

@gz gz referenced this pull request Apr 30, 2018

Open

Testing infrastructure #20

```rust
[testing.framework]
kind = "test" # or bench

This comment has been minimized.

@RalfJung

RalfJung May 2, 2018

Member

The meaning of this flag is not explained anywhere.

This comment has been minimized.

@Manishearth

Manishearth May 3, 2018

Member

looks like this entire section needs to be updated a bit, we removed a bunch of stuff

extern crate proc_macro;
use proc_macro::{TestFrameworkContext, TokenStream};
// attributes() is optional

This comment has been minimized.

@RalfJung

RalfJung May 2, 2018

Member

It is entirely unclear what this comment refers to.

This comment has been minimized.

@Manishearth

Manishearth May 3, 2018

Member

it can be removed, it refers to an older version of the proposal

@djrenren

This comment has been minimized.

djrenren commented Aug 7, 2018

Hey all, I've come up with a simplified proposal and I have an implementation now (though it needs a little cleaning up before a PR).

https://blog.jrenner.net/rust/testing/2018/08/06/custom-test-framework-prop.html

Lemme know what you think!

@TyOverby

This comment has been minimized.

TyOverby commented Aug 7, 2018

@djrenren: This looks really great! One question that I have after a quick scan is

What happens when a test doesn't match the signature provided by the testing provider?

You have this in your example repo

#![test_runner(crate::my_runner)]

fn my_runner(ts: &[&Fn(i32) -> bool]) {
  //...
}

#[test_case]
fn foo(a: i32) -> bool {

}

but I'm interested in what happens if someone puts a regular #[test] fn unit_returning() -> () {...} after your foo test case.

@djrenren

This comment has been minimized.

djrenren commented Aug 7, 2018

So I just tested it out on my implementation compiling this:

#![feature(custom_test_frameworks)]
#![test_runner(crate::my_runner)]

#[cfg(test)]
fn my_runner(_ts: &[&Fn(i32) -> bool]) {
  //...
}

#[test_case]
fn foo(_a: i32) -> bool {
        false
}

#[test]
fn foo2() {

}

and got:

error[E0277]: the trait bound `test::TestDescAndFn: std::ops::Fn<(i32,)>` is not satisfied
  --> src/lib.rs:15:1
   |
15 | / fn foo2() {
16 | |
17 | | }
   | |_^ the trait `std::ops::Fn<(i32,)>` is not implemented for `test::TestDescAndFn`
   |
   = note: required for the cast to the object type `dyn std::ops::Fn(i32) -> bool`

error: aborting due to previous error

Which is an admittedly not very great error message. (though it's also not terrible). I'm definitely open to suggestions here.

@TyOverby

This comment has been minimized.

TyOverby commented Aug 10, 2018

I'd really like to see a way for testsuite authors to write a test suite that provides special support for their custom test suites, but can also run less specific tests.

Here's a few test-runners that I've wanted in the past:

  • A test runner that dumps test results to a rich .html file
  • A test runner that captures stdout and stderr and compares them against a checked-in "baseline" test file.

The 1st of these would need to interact with all kinds of tests (including #[test], #[quickcheck] and more, while the 2nd could optionally pass extra information into the tests to make the production of these files more ergonomic.

@Manishearth Manishearth deleted the Manishearth:post-build-contexts branch Aug 10, 2018

@Manishearth

This comment has been minimized.

Member

Manishearth commented Aug 10, 2018

A test runner that dumps test results to a rich .html file

This is definitely supported, the proposal provides no constraints on output formats (though it suggests we come up with a crate for a standardized format you can choose to use)

A test runner that captures stdout and stderr and compares them against a checked-in "baseline" test file.

Redirecting output from within the same program is tricky, but you can have your test functions return a string and have your test runner recognize it.

@jan-hudec

This comment has been minimized.

jan-hudec commented Aug 10, 2018

@djrenren,

https://blog.jrenner.net/rust/testing/2018/08/06/custom-test-framework-prop.html

Lemme know what you think!

Looks great.

Would it be hard to extract the macro that collects array of annotated items as a separate base feature? I can imagine it could have other uses, perhaps in a dependency-injection framework or some resource collection (it is hard to do as proc macro, because it interferes with incremental compilation).

@TyOverby

This comment has been minimized.

TyOverby commented Aug 10, 2018

@Manishearth the current #[test] test runner manages to capture stdout (and hides it for passing tests).

I think that the point that I'm trying to make is that there are three concepts that people care about configuring:

  1. Coordination of test running and collection of test output. (IDE integrated runners, console runners, rich HTML dump runners, etc..)
  2. Test "enrichers" (quickcheck, expectation tests, etc..)
  3. Custom tests
@Manishearth

This comment has been minimized.

Member

Manishearth commented Aug 10, 2018

So this proposal gives you control over all those, however a single test framework will make all the choices here -- it doesn't give you a way to mix and match output formats and runners.

That said, one of the hopes is to standardize a test runner output crate that has multiple output formats (stdout, json, perhaps html). Most test frameworks can just use this and expose the same options, and the json side can be plugged into everything else.

the current #[test] test runner manages to capture stdout (and hides it for passing tests).

ah, looks like io::set_print is a thing.

@djrenren

This comment has been minimized.

djrenren commented Aug 10, 2018

@jan-hudec Yeah such a thing would be possible, but it's also probably not a great thing to rely on. I believe it would have negative effects on incremental compilation, as well as violate the structure of the language. All told it's a scary enough change to not be included as part of this work, but by all mean throw up an RFC. It's definitely possible.

@CAD97

This comment has been minimized.

CAD97 commented Aug 10, 2018

@TyOverby @Manishearth

Just to have it said in this thread as well,

Stdout/stderr capture is kind of messy. The current framework for tests a) only works on one thread and b) only works with print!. If you use io::stdout, it will bypass the capture.

If you go check the tracking issue I went into more depth there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment