Implement test environments and before/after closures #30

regexident · 2017-08-18T09:47:38Z

consider this the first in a series of PRs aiming to improve rspec, as layed out in tracking issue #29.

It implements a shared context/example environment with mutability isolation, which aims to enable sophisticated test scenarios as well as hopefully allowing us to safely implement concurrent test execution without having to worry about environment corruption, thanks to the power of Rust's borrow checker. (as such it aims to fix issue #7)

Further more it adds support for specifying multiple before_all (aka before), after_all (aka after) closures (with guarantees of preserved execution order) to be executed prior/posterior to execution of a context's context or example members. (as such it aims to fix issue #20)

Last but not least it adds support for specifying multiple before_each, after_each closures (with guarantees of preserved execution order) to be executed prior/posterior to execution of each of a context's context or example members. (as such it aims to fix issue #8)

Please note that during the Runner's complete overhaul I removed its unit tests at some point and haven't yet found the time to add them back. So consider this a WIP, I guess?

Anyway, I'd love to hear your feedback.

Ps: I also upped the Rust version on TravisCI from 1.9 to 1.18, for the sake of being able to use pub(crate) and other neat things. I hope that's fine with you?

Pps: Also expect the next PRs to be more finely grained. 😉

…PACE)

mackwic · 2017-08-18T20:46:29Z

Cargo.toml

+name = "rspec"
+readme = "README.md"
+repository = "https://github.com/mackwic/rspec"
+version = "1.0.0-beta.3"


this would be a .4

Followed by hopefully quite a bunch of betas and a final RC, yeah.

mackwic · 2017-08-18T20:47:06Z

Cargo.toml

+
+# [dependencies.expectest]
+# optional = true
+# version = "0.8.0"


remove dead code

Yeah, this would have to be bumped to 0.9.1 and the other git branch one removed.

mackwic · 2017-08-18T20:47:18Z

Cargo.toml

+branch = "consistency"
+
+# [dev-dependencies]
+# expectest = "0.8.0"


dead code to remove

mackwic · 2017-08-18T20:50:21Z

Cargo.toml

+
+[dev-dependencies.expectest]
+git = "https://github.com/regexident/expectest"
+branch = "consistency"


You shouldn't need that

mackwic · 2017-08-18T20:51:08Z

Cargo.toml

+[dependencies.expectest]
+optional = true
+git = "https://github.com/regexident/expectest"
+branch = "consistency"


I believe your PR has been merged in the v0.9.1 ? (congrats btw)

Thanks. expectest + rspec are a beautiful match, imho. 😉

mackwic · 2017-08-18T21:14:48Z

src/example_report.rs

+
+impl Failure {
+    pub fn new<S>(message: Option<S>) -> Self
+        where String: From<S>


Where String implements From<S> ? Why not S : ToString ?

Sure. To be honest I didn't have ToString on my screen at all, when I wrote this. 😇

To be frank, it's not the same. There is no blanket impl String : From<T: ToString> so it you depends on this bound, it will break something.

Where do you use this ?

It is used for Failure creation from either a str (literal) or a String (format!(…)).

mackwic · 2017-08-18T21:15:02Z

src/example_report.rs

+}
+
+// #[derive(Clone, PartialEq, Eq, Debug)]
+// pub struct ExampleReport(Result<(), String>);


dead code to remove

mackwic · 2017-08-18T21:17:05Z

src/formatter/simple.rs

-                if result.is_err() && !self.name_stack.is_empty() {
-                    let failure_name = self.name_stack.join(" | ");
-                    self.failures.push(failure_name);
+    fn exit_suite(&mut self, report: &ContextReport) {


too much logic here

I have a second PR almost ready however which completely parallelizes the entire test suite execution (configurable) via rayon. It completely messes up the console log for obvious reasons. With this in mind I'd suggest to just ignore this one for now when deciding when and how to merge this PR, as I plan to rework this part (and add unit tests) in the very next PR to follow. But for the sake of clarity I'd rather not any further major refactors to this PR.

The next PR also turns the entire suite/context/example hierarchy immutable and makes the runner execution immutable (as in …(&self, …), plus some Arc<Mutex<…>> on the EventHandlers) as well. This whole immutability thing is what enables the fearless concurrency cough to begin with.

As I said, I am OK with a complete big PR. Sometimes refactoring are large and it's OK.

Why don't you push it all here, and we work the rest together ?
At least please place block comments that signal where the moving code is, but I suspect a change in type might break your next PR...

mackwic · 2017-08-18T21:21:03Z

src/runner.rs

+    pub fn run(mut self) -> ContextReport {
+        let suite = mem::replace(&mut self.suite, None).expect("Expected context");
+        panic::set_hook(Box::new(|_panic_info| {
+            // silently swallows panics


this should fail the test, no ?

You mean the // silently swallows panics? Oh that just prevents the panic from getting printed to the console.

It's this that does the actual fail catching.

Oh ! Okay, so then maybe change the comment to: // XXX panics already catched at the test call site, don't output the trace in stdout or something equivalent ?

( I like the convention XXX == weird behavior if you are new to the code base)

mackwic · 2017-08-18T21:21:34Z

src/runner.rs

-            result = test_res.or(result);
+    pub fn run_or_exit(self) {
+        if self.run().failed > 0 {
+            ::std::process::exit(101);


I basically checked what $ cargo test returns for a failing test (via $ echo $?). Turns out it's 101.

https://www.reddit.com/r/rust/comments/3qdvez/guarantees_about_exit_codes/cwf46cb/

Excellent ! I suggest you put this link in comment just before the process::exit line. Something like that:

pub fn run_or_exit(self) { if self.run().failed > 0 { // XXX Cargo test failure returns 101. Ref: https://www.reddit.com/r/rust/comments/3qdvez/guarantees_about_exit_codes/cwf46cb/ ::std::process::exit(101);

mackwic · 2017-08-18T21:27:43Z

There is tremendous work here, I can see it. I it is impressing. And I quite like the look of it !

this was a quick look at the code, I need to load it in my IDE and play with it a bit to suggest modifications. This weekend I'll make better remarks.

regexident · 2017-08-19T00:05:04Z

There is tremendous work here, I can see it. I it is impressing. And I quite like the look of it !

Awesome, thanks!

this was a quick look at the code, I need to load it in my IDE and play with it a bit to suggest modifications. This weekend I'll make better remarks.

Cool. As already mentioned in one of the comments I have another PR in the works (as a follow-up for this one) that turns the whole thing into a parallel executed test Runner that walks over a strictly immutable hierarchy of suites/contexts/examples with the only mutable thing being the environment that is being passed around safely and with strict mutability isolation per context.

For all this I need to completely overhaul the formatter. And while I'm doing that I also plan to add back the unit test that I had to remove from runner and formatter.

mackwic · 2017-08-19T07:58:33Z

src/runner.rs

+            // silently swallows panics
+        }));
+        self.visit(&suite);
+        let _ = panic::take_hook();


this is not clear and missed it at first review. Add a comment // XXX unregister panic hook ?

…etc.

regexident · 2017-08-19T16:56:30Z

I just pushed the WIP that makes the entire test execution immutable (with exception of the environment that's getting passed around and forked).

It further more adds support for using a single runner with multiple suites, and removes lots of debris from the past, such as unused explicit lifetimes and mut markers.

The formatter's output breaks when run in parallel mode with Configuration::default() (i.e. parallel = true). I hope to find time tomorrow to tackle this by overhauling the hole formatting thing.

Please feel free to point out any other parts that still need some work to get merged.

mackwic · 2017-08-20T12:40:22Z

src/context_member.rs

+}
+
+unsafe impl<T> Send for ContextMember<T> where T: Send {}
+unsafe impl<T> Sync for ContextMember<T> where T: Sync {}


these two needs XXX comment on why they are here

Necessary for parallelization. Good point, will document.

mackwic · 2017-08-20T12:42:09Z

src/example.rs

+}
+
+unsafe impl<T> Send for Example<T> where T: Send {}
+unsafe impl<T> Sync for Example<T> where T: Sync {}


same here, need an explanation in comment

Necessary for parallelization. Good point, will document.

mackwic · 2017-08-20T13:24:35Z

src/runner.rs

-        self.handlers.broadcast(&Event::FinishedRunner(result));
-        result
+impl Runner {
+    pub fn run<T>(self, suite: (Suite<T>, T)) -> SuiteReport


I don't understand the difference between run and run_with

(same for run_and_exit and its run_and_exit_with counterpart)

One takes an explicit configuration, one uses the implicit default.

However given that test initiation has become quite cumbersome with the latest changes it doesn't make much a change, I guess. Maybe just drop the default config one and remove the _with from the explicitly configured variant.

My plan is to at some point (i.e. once the overall architecture has settled) re-introduce some API akin to rdescribe(…), which would wrap all this:

let simple = rspec::formatter::Simple::new(io::stdout()); let formatter = Arc::new(Mutex::new(simple)); let configuration = Configuration::default(); // .parallel(false); let runner = Runner::new(configuration, vec![formatter]); #[derive(Clone, Debug)] struct Environment { // … } let environment = Environment { // … }; runner.run_or_exit(rspec::given("a BTreeSet", environment, |ctx| { // … }));

into something more palatable, like this:

#[derive(Clone, Debug, Rspec)] struct Environment { // … } rspec::run_or_exit(rspec::given("a BTreeSet", Environment { // … }, |ctx| { // … }));

mackwic · 2017-08-20T13:35:58Z

Are you OK if I push a Rustfmt commit on this branch ?

For the next changes modifications, I'd like to make PRs on this PR.

regexident · 2017-08-20T14:08:19Z

Are you OK if I push a Rustfmt commit on this branch ?

Oh please, do. 🙂

For the next changes modifications, I'd like to make PRs on this PR.

Fine with me. I don't plan to do any major changes on this one, other than cleanups.
Unless you want me to push the reworked Formatter (to make it work with parallel test runs) here as well, before merging. Up to you. 🙂

I'm currently looking into the concurrent formatter thing.

mackwic · 2017-08-20T14:10:43Z

Great !

regexident · 2017-08-20T14:54:47Z

So, anything in particular you'd like me to fix prior to merging?

mackwic · 2017-08-20T15:26:01Z

Yes, there is a lot of work to be done in this PR before being ready to merge. Documentation, tests, examples are the obvious one. I also saw some bigger changes but I am not sure I understand how it could break the rest (like duplication). I am currently playing with it in my IDE to test changes.

When it's a little thing I can put a comment. But when it's a big change, as I already tested it in my IDE, I make a change proposal via external PRs (eg: #31 and #32 ), all dependants PRs needs to be merged before merging this one. It's like a comment on your diff, but I already tested the code.

The goal is to keep the debate contained in little changes and improve progressively. You can disagree on my proposal (it's fine !), or have better ideas on how to do it. When we agree, we merge, we move forward, and we have better code. 👌

Is this fine by you ?

regexident · 2017-08-20T16:10:48Z

Yes, there is a lot of work to be done in this PR before being ready to merge. Documentation, tests, examples are the obvious one. I also saw some bigger changes but I am not sure I understand how it could break the rest (like duplication). I am currently playing with it in my IDE to test changes.

Good point. I will add a couple more examples and extend the existing documentation.

When it's a little thing I can put a comment. But when it's a big change, as I already tested it in my IDE, I make a change proposal via external PRs (eg: #31 and #32 ), all dependants PRs needs to be merged before merging this one. It's like a comment on your diff, but I already tested the code.

Cool.

The goal is to keep the debate contained in little changes and improve progressively. You can disagree on my proposal (it's fine !), or have better ideas on how to do it. When we agree, we merge, we move forward, and we have better code. 👌

👍

Is this fine by you ?

Absolutely!

move Context modules in a directory

mackwic · 2017-08-27T15:37:49Z

src/runner/mod.rs

+        fn it_can_be_called() {
+            // arrange
+            let runner = Runner::default();
+            let block = Block::Example(Example::new(ExampleHeader::default(), |_| ExampleReport::Success));


seems like a bug to me (or at least a regression).
This wont compile:

let block = Block::Example(Example::new(ExampleHeader::default(), |_| ()));

It should !

Same for false:

/Users/thomas/.cargo/bin/cargo test --color=always --package rspec --lib runner::tests::impl_visitor_block_for_runner -- --nocapture Compiling rspec v1.0.0-beta.4 (file:///Users/thomas/projects/rust/rspec) error[E0308]: mismatched types --> src/runner/mod.rs:607:83 | 607 | let block = Block::Example(Example::new(ExampleHeader::default(), |_| false)); | ^^^^^ expected enum `report::example::ExampleReport`, found bool | = note: expected type `report::example::ExampleReport` found type `bool`

No, this is correct. Example::new(…) takes a closure which handles the conversion from ()/bool/… to ExampleReport. One could use F: 'static + Fn(&T) -> impl Into<ExampleReport> instead of F: 'static + Fn(&T) -> ExampleReport in future, but that's not available yet. Also Example::new(…) is not intended for public use anyway. That's what ctx.then(…) is for.

However if you see a way to make this cleaner I'm all for it. :)

regexident · 2017-08-27T17:15:33Z

Some theoretical remarks:

Lemme take the opportunity to thank you for your great and detailed comments! They helped a lot! At some point it can become difficult to see the forest for the trees.

Runner is a pure dispatcher (and this is great !), it doesn't have state per se

Yeah, this was one of the main goals of my overall concept. Suite/Context/Example are pure, too.

So we only need to test the order of the dispatch and the eventual logic behind (like conditional dispatch)

Yes.

I believe your test do too many things at once. I find that simpler, little focused tests seems easier to write, maintain, and read IMO

True. Your tests look good! 👍

It's cool the API is generic, I like doing impl for testing needs and inject my little stubs :)

😎

I am impressed by the overall design. It's good, really.

I'm glad you like it!

I already have timing implemented (yet will have to rebase it and probably rewrite some parts of it now). And filtering/focussing shouldn't be too difficult to implement now either.

regexident · 2017-08-31T13:57:30Z

With 97cab2b Logger has now reached a state where I'm happy with both implementation and API use.

That being said I plan to add realtime progress for parallel execution via the indicatif crate with a later, separate PR.

What is your take on the progress of this PR? What's still missing? What should I fix?

mackwic · 2017-08-31T15:07:10Z

LGTM ! I think this PR is now in an acceptable state :)

regexident · 2017-08-31T15:30:08Z

Great! Feel free to rebase and merge it at your discretion then.

Thanks for the ride, @mackwic! 😎

mackwic · 2017-09-01T08:59:32Z

🎉

regexident · 2017-09-01T12:45:28Z

Cool, but sure you want this massive PR to be squashed into a single commit?

I think for a massive change of API/implementation like this it would be more than useful to have a more finely-grained log of changes, no? It would also give a better picture of the activity of this project, which with a single commit might give the false impression of still being in some kind of stagnation.

mackwic · 2017-09-01T16:12:38Z

I don't want to optimize by the number of commits. It's true, some people look at it but it's a meaningless metric.

I want this PR to be squashed so that we can't checkout an intermediate state from master.

regexident · 2017-09-01T16:56:14Z

I don't want to optimize by the number of commits. It's true, some people look at it but it's a meaningless metric.

Fair point. I personally don't care either.

I want this PR to be squashed so that we can't checkout an intermediate state from master.

Ah I see, this makes sense.

regexident added 4 commits August 15, 2017 17:21

Add support for test context environments

2c466e9

Move environment into Runner (changing strategy from BFS to DFS in PS…

f69294c

…PACE)

Add support for before and after (and variants) closures

b25a374

Clean up project structure

f8be924

This was referenced Aug 18, 2017

Add before_alland after_all Context methods #28

Closed

Tracking issue for current refactoring efforts #29

Open

mackwic reviewed Aug 18, 2017

View reviewed changes

mackwic reviewed Aug 19, 2017

View reviewed changes

regexident added 2 commits August 19, 2017 12:25

Make the whole thing parallel in execution.

5a10889

Make runner work with multiple suites, remove unnecessary lifetimes, …

f0b1aa6

…etc.

Rustfmt the files + move runner::Configuration in its own module

020d2b2

regexident mentioned this pull request Aug 20, 2017

Async tests #19

Open

mackwic reviewed Aug 20, 2017

View reviewed changes

mackwic mentioned this pull request Aug 20, 2017

[Environment] little Runner refactorings #31

Merged

twickham added 2 commits August 20, 2017 16:44

move Context modules in a directory

95812d3

cargo fmt

e67b7f4

mackwic and others added 5 commits August 20, 2017 20:37

Merge pull request #32 from mackwic/extract_context_to_module_dir

6feb165

move Context modules in a directory

Extract private method and rename inner variables

4e7b0c3

remove useless _with methods in Runner

1c08f99

Remove need for rayon::initialize

a54bbd6

Remove unused/commented code from example

091cfa5

twickham added 2 commits August 27, 2017 17:24

add Runner tests for wrap_all and Drop trait

f2f515c

Add first test for impl_visitor_block_for_runner

9ca53f4

mackwic requested changes Aug 27, 2017

View reviewed changes

regexident added 4 commits August 27, 2017 17:59

Fix macro, allowing passing of expression, not just identifiers

09cd99d

Fix missing new-line in formatter

eb546ba

Simplify ‘simple’ example

6a7401b

Replace rspec_run! macro with function

137be3e

regexident force-pushed the environment branch from 455da72 to 137be3e Compare August 27, 2017 16:51

Some more cleanup

2519af8

twickham and others added 4 commits August 28, 2017 08:07

Fix test runner::impl_visitor_block_for_runner::it_can_be_called

c30782f

Add tests for impl_visitor_example_for_runner

8e3ee5e

Rename ‘Formatter’ to ‘Logger’

9b9bbbf

Improve logger, merging serial & parallel logger

97cab2b

regexident force-pushed the environment branch from 7d44ee0 to 97cab2b Compare August 31, 2017 13:54

mackwic approved these changes Aug 31, 2017

View reviewed changes

Fix documentation for rspec::run(…) function

97b2f62

mackwic merged commit f543f9f into master Sep 1, 2017

mackwic deleted the environment branch September 1, 2017 08:58

This was referenced Oct 4, 2017

Timing the test suite #10

Closed

Real before_each and after_each #8

Closed

Implementing let (or some kind of state for tests) #7

Closed

Implement before_all and after_all #20

Closed

Implement test environments and before/after closures #30

Implement test environments and before/after closures #30

Conversation

regexident commented Aug 18, 2017 • 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

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

mackwic Aug 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mackwic Aug 19, 2017 • edited Loading

Choose a reason for hiding this comment

mackwic commented Aug 18, 2017 • edited Loading

regexident commented Aug 19, 2017

Choose a reason for hiding this comment

regexident commented Aug 19, 2017

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

mackwic commented Aug 20, 2017

regexident commented Aug 20, 2017

mackwic commented Aug 20, 2017

regexident commented Aug 20, 2017

mackwic commented Aug 20, 2017

regexident commented Aug 20, 2017

Choose a reason for hiding this comment

mackwic Aug 27, 2017 • 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

regexident commented Aug 27, 2017 • edited Loading

regexident commented Aug 31, 2017

mackwic commented Aug 31, 2017

regexident commented Aug 31, 2017 • edited Loading

mackwic commented Sep 1, 2017

regexident commented Sep 1, 2017

mackwic commented Sep 1, 2017

regexident commented Sep 1, 2017

regexident commented Aug 18, 2017 •

edited

Loading

mackwic Aug 19, 2017 •

edited

Loading

mackwic Aug 19, 2017 •

edited

Loading

mackwic commented Aug 18, 2017 •

edited

Loading

mackwic Aug 27, 2017 •

edited

Loading

regexident commented Aug 27, 2017 •

edited

Loading

regexident commented Aug 31, 2017 •

edited

Loading