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

Improved spec library #167

Closed
booch opened this issue Jul 21, 2014 · 16 comments
Closed

Improved spec library #167

booch opened this issue Jul 21, 2014 · 16 comments

Comments

@booch
Copy link
Contributor

booch commented Jul 21, 2014

Our Spec class could use some improvements, to bring its feature set closer to something like RSpec:

  • Something similar to RSpec's let and let!
  • Something similar to RSpec's before blocks to help with setup
  • Something like RSpec's subject
  • Make it easier to define custom expectations (matchers)
  • Isolate expectation methods (eq, be_true, etc.) so they don't pollute global method namespace
  • Something similar to RSpec's newer expect syntax, or Hamcrest matchers' assertThat syntax, so we don't have to pollute Object's namespace

These will likely require a major overhaul to the Spec class. It might make sense to replace the class instead.

@booch
Copy link
Contributor Author

booch commented Jul 21, 2014

I just created this issue to let people know that I'm working on this.

@asterite
Copy link
Member

I agree. But how do you plan to implement this? I was thinking a macro for describe and make it keep the block in a list for later execution.

I like the way you do assert in Elixir and Rust. We can do the same in Crystal:

macro assert_eq(expected, actual)
  unless {{expected}} == {{actual}}
    raise "assertion `#{ {{expected.stringify}} } == #{ {{actual.stringify}} }` failed: #{ ({{expected}}).inspect } != #{ ({{actual}}).inspect }"
  end
end

assert_eq 2 + 2, 5

It's super clean (the usage, not the macro :-P) and the error you get on assertion failure is this:

assertion `2 + 2 == 5` failed: 4 != 5

One step further would be to be able to do:

assert 2 + 2 == 5

and have macros pattern match that (so you can consider <, >, etc.), but for now it's too much (it would be a whole new feature).

On the other hand, RSpec is also nice. The lengths are almost the same:

assert 2 + 2 == 5
(2+2).should eq(5)

and the matchers are all defined in Crystal, not in macro land. So I have mixed feelings about this :-)

What's the problem with polluting Object? It's just should and should_not, and they are only "polluted" in specs.

@booch
Copy link
Contributor Author

booch commented Jul 21, 2014

@asterite I'll probably need to make describe a macro to make let work. I haven't gotten that far yet, although I have done some thinking about it. (I think @waj and I talked about it some on IRC.) I'm working on a more OOP way to do the describe and it blocks first. I've gone back and forth on whether to keep the it blocks until the end, or run them immediately. I don't think it really matters though.

The problem with adding should comes up with proxy objects. See this explanation. I'm not sure if Crystal might have that issue, but would rather not be based on someone else's deprecated syntax. I also find the should syntax weird with regard to the space after should and should ==.

So I was thinking of something like Hamcrest matchers, but a bit more flexible using the builder pattern. Something like this:

verify_that(1).is(1)
verify_that(1).equal_to(1)
verify_that(1).is.equal_to(1)
verify_that(1).is_not(2)
verify_that(1).is_not.equal_to(2)

It's a bit more verbose, but reads better than even the new RSpec expect syntax. I find it to be about as readable as the should syntax, without the issues mentioned above. I hate Java, but the Hamcrest matchers made testing in Java pretty nice. I've also used the Hamcrest matchers in Python, and it looks like there's a version for Objective C now. In Ruby/Crystal, we can improve on the Hamcrest syntax by using the builder/chaining/fluent pattern instead of nested calls.

Yesterday I realized that with macros we could probably do something close to what you mentioned, showing the source as well as the results. This would work well with most kinds of matchers. I don't think we could get to the flexibility of allowing assert 2 + 2 == 5 though, as it would require a full Crystal parser in the assert macro.

@PragTob
Copy link
Contributor

PragTob commented Jun 7, 2015

Hi @booch could you maybe make the list into a check list (e.g. - [ ] and - [x]) so it is easier to track what has already been done? E.g. I see there's already before_each.

Thanks!
Tobi

@waterlink
Copy link
Contributor

@PragTob before_each implementation is global currently.

That is because currently there is no scoping of describe and context, and examples are executed as soon as encountered. Effectively, describe and context just modify example description prefix and execute their block, and it appends last description to example and executes its block. All of them just normal global methods.

Because of that before, after, let, let! are not achievable. At least not to the functionality similar to RSpec.

To achieve that (+ additionally random order example execution), we will need to define examples inside of context without actually executing them, and we need to remember all calls to before, after, let, let! and determine which example group they are in. After that we actually run all examples with all before, after, let, let! and subject applied to their respective example groups.

This can be achieved easily in the same way RSpec does it - class inheritance. I tried doing so and compilation of compiler and stdlib takes ridiculously long time, as well as test suite run. RAM consumption is much higher too. This is actually expected, since it generates tons of classes inheriting from each other.

Other way, will be not that simple, but still not very complex: describe and context create some sort of Context object and set it to currently active globally (or thread-local?) while running provided block. it, let, before and friends register their respective blocks to the current context. Then Runner jumps in, it will take care about execution order of these blocks. This will definitely allow to make before and after possible and scoped to proper example group, but what about let and subject? - since we are not in derived class here, we can't just define method with that name - we need some smarter way of making these names available to example, and even making them lazy..

@ysbaddaden
Copy link
Contributor

Just for reference: the describe/before/after/let macros to generate test classes/methods with inheritance are implemented in my minitest port. I didn't bench compile time and memory usage thought, but the more classes there are, the slower crystal compilation gets, so I'd expect it to get slower over time with many nested describes and let overrides.

@asterite
Copy link
Member

I'll close this. spec works fine. If you want a before hook, define a private method and invoke it. That way specs are also easier to understand and there's no magic. And expect(...).to ... is a bit more verbose than should and I kind of dislike it (had to use this for some time in a Rails 4 project and it's so tedious).

Alternatively, use minitest.cr if you need other funcionality, crystal spec works fine with that too.

@PragTob
Copy link
Contributor

PragTob commented Sep 30, 2015

@asterite one of my main gripes with spec is also that all of the methods are implemented on a global level afaik. Are you open for improvements in that regard?

@asterite
Copy link
Member

@PragTob Did you find any conflicts so far?

@PragTob
Copy link
Contributor

PragTob commented Sep 30, 2015

@asterite no, I just believe that no library should define methods on the main scope. I never know what a library will be used for, and maybe some application wants to define methods like eq and be_true and it wants to use spec for tests. Or.. afaik describe and it . Polluting name spaces like that is not good imo.

@asterite
Copy link
Member

@PragTob You mean something like this?

The "problematic" methods are should and should_not. But if you write specs (and you probably do) you won't use those names in your objects. Same as you wouldn't use object_id for your objects, or define a same? method, or a hash method that doesn't return a hash code, etc.

I believe (and you said "I just believe") this is more of a philosophical issue rather than a concrete one. For the time RSpec had them as globals, nobody complained and it worked fine. Then they had issues with BasicObject and blank slates and many things that just don't exist in Crystal, so they changed it.

We also define to_json for all objects, but nobody seems to complain about that. (EDIT: this is actually not true, but you can think that the to_json name is taken and you won't be able to use it with another meaning in your library)

@PragTob
Copy link
Contributor

PragTob commented Sep 30, 2015

@asterite no I'm not talking about should/should_not - not a big fan of those but I agree that it is nicer to type than the expect way. Also, with crystal featuring method overloading we could type them appropriately and as long as no one would define a should method that takes an Expectation as well, then we're all ok and this actually works.

I'm talking about all the matchers (eq, be_true) being defined on the main object/default scope (as well as describe, it and so on). So for instance be_true and eq. I.e. more like this - which causes a compiler error due to the conflict.

Alltogether that are ~20 methods defined on the main object/global scope that no one using spec can define, without potentially messing with spec (granted that through method overloading, this will work a lot of times as opposed to ruby).

@schmurfy
Copy link

On ruby I am still running with bacon both for personnal and work projects and I mostly ignored the trend about rspec (which for me provides a lot of useless features for my needs), bacon let me write this:

should 'add numbers' do
  add(1, 4).should == 5
end

compared to what crystal currently has:

should 'add numbers' do
  add(1,4).should eq(5)
end

Only a should method is defined on Object which never conflicted with anything I used until now (roughly 7 years) and allows using existing methods, in addition I find the first to be more readable than the later but it might just be me.

@georgeu2000
Copy link

I too was looking for let syntax...

@RX14
Copy link
Contributor

RX14 commented Oct 5, 2017

The best way to do things you'd do with let and before/after is to either use helper methods to abstract creating objects, or use a helper method with yield to surround your spec code with code which is before and after. Here's a good example of helper methods and their usage: 1 2

@alexanderadam
Copy link

@georgeu2000 well, @waterlink got you covered with spec2.cr.

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

No branches or pull requests

10 participants