[RFC] Spec: add a way to check that a method's value is truthy or falsey

[asterite]

Right now we can do this:

describe Array do
  it "is empty" do
    ary = [] of Int32
    ary.empty?.should be_true
  end
end

That's fine but we could improve this situation.

In RSpec one can do:

ary.should be_empty

This works by letting be_empty use method_missing and creating a matcher on the fly. This can't work in Crystal, not because of method_missing but because the generated matcher can't be aware of what's the target object being sent to it.

Some ideas that were proposed in another PR, starting from #7500 (comment) are to be able to do something like this:

ary.should &.empty?

I pointed out that the readability of that can be improved a bit:

ary.should_be &.empty?

But when I went ahead to implement that I noticed in specs we have, for example:

File.exists?(...).should be_true

Replacing that with should_be looks awkward:

File.should_be &.exists?(...)

So I had that idea that we could have assert and refute, similar to other languages (but it's not the same as those other languages).

Essentially, the above two examples would be:

ary.assert &.empty?
File.assert &.exists?(...)

Then we could also do:

[1, 2, 3].refute &.empty?

One downside of using those names is that they don't have "should" in them. When looking at some specs right now the assertions are easy to spot because they almost always have "should" in their names (the exception being expect_raises).

But maybe it's fine? assert and refute are very convenient methods to have, to avoid the should be_true boilerplate.

The implementation is pretty straight-forward:

require "spec"

module Spec
  module ObjectExtensions
    def assert(file = __FILE__, line = __LINE__)
      fail "Assertion failed", file, line unless yield self
    end

    def refute(file = __FILE__, line = __LINE__)
      fail "Assertion failed", file, line if yield self
    end
  end
end

describe Array do
  it "is empty" do
    ([] of Int32).assert &.empty?
  end

  it "is not empty" do
    [1].refute &.empty?
  end
end

Thoughts? Any other names we could use?

[Blacksmoke16]

FWIW you can already do arr.should be_empty.

EDIT: We also have val.should be_truthy and val.should be_falsey as well.

[asterite]

Now that I think about it, this could also work as a top-level method:

assert ary.empty?
assert File.exists?(...)

which is easier to spot and easier to read and write.

[konovod]

be_empty is so much better than a.empty?.should be_truthy because in first case you get expected [1, 2] to be empty message instead of expected true, got false. So it is much easier to understand what exactly gone wrong. If assert will keep this property (won't just fail, but will print failed object), then it would be definitely better than current state.

[Blacksmoke16]

Sorry I mean that you can already do that.

https://play.crystal-lang.org/#/r/832l

IMO, I find File.exists?(...).should be_true to be sufficient, especially if/when more descriptive expectations exist for a given case, such as if an array is/is not empty (or if a hash/string is empty).

Having a whole different syntax/structure just for this seems a bit overkill.

[oprypin]

Are you really gonna tease assert now...
Of course it should be assert, everything should be assert rather than this entirely new silly language.
Yes, should be_true is ugly, as is everything else about spec. One can make the exact same argument.
#3095

[asterite]

Hehe, you are right. So bad macro code is slow and we can't use it for this, but it seems I just proposed doing that :-)

I guess there's no need to improve this right now.

[straight-shoota]

I might've spiked this discussion, but I'm a bit skeptical. Seems like I'm not the only one.

a.should_be &.b? might be better than a.b?.should be_true and it certainly improves developer feedback. It shows the actual value that failed the spec, not that the result of the call that was false when true was expected, which doesn't help anything to understand for which value it failed.
But while showing the actual value, it still fails to report which property on that value failed. That would be a selling feature. But I doubt there's a way to achieve a generic solution for this.

But then there are a few hard coded matchers, like the comparison operators or be_empty, contain, {start,end}_with, match. They show both the actual value and the predicated used in the expectation. They're obviously not flexible, you can't just use custom predicates ad-hoc. But they offer the best results. And they probably cover most use cases, so customizability doesn't provide too much additional value.

[asterite]

Well, with a generic assert macro that could introspect the call we could theoretically show as much info as we'd like:

assert ary.empty?
# Failure: Expected [1, 2, 3] to be empty

The problem is that doing such macro expansion for each and every assert in a big test suite takes a considerable amount of time.

Maybe I should create such macro and use it a lot and benchmark the compiler to see where that time is going... if there's a way to optimize it, it can be a solution.

[asterite]

So this is interesting... I did the above test. I tried it with this (dummy) program:

require "spec"

macro assert(call, file = __FILE__, line = __LINE__)
  {% obj = call.receiver %}
  {% name = call.name %}
  %obj = {{obj}}
  %call = %obj.{{name}}
  unless %call
    fail "Expected #{%obj} to be {{name[0...-1]}}"
  end
end

describe "Foo" do
  {% for i in 0..12_000 %}
    it "bar {{i}}" do
      a = [1, 2, 3]
      assert a.empty?
    end
  {% end %}
end

Profiling it I can see most of the time, 12s, is gone in Crystal::SemanticVisitor#expand_macro, and half of that time (6.91) is gone on Set#dup. Wat?? Looking at Set#dup, it seems it's doing Set.new(self), which basically populates the Set from its values, but it would be so much more efficient to dup the internal Hash and use that to construct the set.

I hope to find many more optimizations, maybe we can optimize macros a lot more!

[asterite]

I'll reopen because we might end up having an assert macro or something similar depending on whether we can optimize macros.

[asterite]

Welp, I was able to optimize this case particular case (for which there was something really bad going on) down to 0.4s :-)

I'll send a PR soon.

Next up: someone (or me, if I have time, but I doubt it) could try writing that fancy assert macro and trying it out in the snippet above. The idea is to implement #3095 and test this snippet but using the compiler that will have the performance optimizations. If that compiles fast, at least in similar times to using should, then I'm all for it.

[oprypin]

References to my prior implementations of power assert:

• https://github.com/oprypin/spec-assert/blob/master/src/spec_assert.cr (macro, still works!)
• https://github.com/oprypin/crystal/commits/feature/assert (old compiler fork)
• https://gist.github.com/b4f150b0d22e9e3ee2dff3bcf8ec8cb5/ (old external tool)

[asterite]

Nice!

I just tried it with this code:

require "spec"

describe "Foo" do
  {% for i in 0..15000 %}
    it "bar" do
      a = 1
      b = 2
      assert a == b
    end

    it "baz" do
      a = 1
      assert a.is_a?(String)
    end
  {% end %}
end

It takes about 20 seconds to compile. If I use should instead of assert it takes 16 seconds. It's not a big difference, I guess mainly because assert generates more code than a simple call, but it could be improved by extracting stuff to a method.

I think I like it.

That said, I can't see how this can be extended. For example if I do:

assert [1].empty?

it says "expected false to be truthy". Maybe for this specific case we can change it to be "expected [1] to be empty", if it's a bang call with no arguments. Maybe that's enough.

Then there's assert "foo".start_with?('b'), the message should be 'expected "foo" to start_with 'b''... I guess? But all of these cases have to be baked in into the assert macro. If it could somehow be extensible that would be great... maybe by adding to a special spec constant at compile-time.

Thoughts?

[straight-shoota]

The should DSL explicitly encodes which value is being tested: The one that receives #should call.
This doesn't work with assert when you have commutative expectations. When testing by comparison, you could do either of those:

assert a == b
assert b == a

Both expressions should be equivalent. But which value is the actual and which the expected?
By convention, we could declare the first value as actual. But it's not as explicit.

It's similar with testing a property vs. testing with a predicate.

a.should_be &.foo?    # test `a` using predicate #foo?
a.foo?.should be_true # test property `a.foo?`

While both tests look similar, the should clearly separates the actual value from whatever test is applied.

With assert, this is not as obvious. It could be expressed for example like this:

assert a.foo?         # test `a` using predicate #foo?
assert a.foo? == true # test property `a.foo?`

Both variants seem equivalently interchangeable, and the second one unnecessarily verbose. The implicit semantics would not be directly visible. So again, different semantics would only be based on convention.

Relying on convention is not a critical stopper, but a thing to consider. Explicitness is always better.

[vlazar]

assert a == b
assert b == a

Both expressions should be equivalent. But which value is the actual and which the expected?

Good point.

It can be the same order as arguments order used in assert_equal() implementations in other test frameworks. If ALL other frameworks agree on the the arguments order, then it's quite established convention we can rely on.

[RX14]

Is Expected [1] to be empty really that much easier to read or understand than assertion `[1].empty?` failed?

The problem comes when you have an expression like foo.empty? and you don't know what foo is.

So how about:

Assertion `foo.empty?` failed

foo # => [1]

You could pretty print the call args too, if any.

That is, take the top-level call only, take the name of the receiver and all arguments, and pretty-print them. It'll end up being a pretty simple, fast, macro, I think. Since most of the work is just done by the pretty printer.

[RX14]

The should DSL explicitly encodes which value is being tested: The one that receives #should call.
This doesn't work with assert when you have commutative expectations. When testing by comparison, you could do either of those:

I really can't imagine a spec scenario where what's being tested isn't obvious from the spec name and the body combined. You can write bad code in specs as well as application code, but I don't think that's a good point against assert.

[RX14]

I think this is pretty much 95% of the way to how I'd like the assert macro to work: https://carc.in/#/r/8515

[Sija]

Solution proposed by @asterite in the description seems to me like a valuable addition to the Spec library. I'm wondering whether we could adopt it just as it is?

%w(soo nice).refute &.empty?
# vs
%w(not so nice).empty?.should be_false

[jhass]

I'm already not a big fan of polluting Object with should and should_not, adding more methods is only making that worse.