Open asterite opened 4 years ago
FWIW you can already do arr.should be_empty
.
EDIT: We also have val.should be_truthy
and val.should be_falsey
as well.
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.
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.
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.
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.
https://github.com/crystal-lang/crystal/issues/3095
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.
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.
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.
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!
I'll reopen because we might end up having an assert
macro or something similar depending on whether we can optimize macros.
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.
References to my prior implementations of power assert:
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?
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.
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.
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.
The
should
DSL explicitly encodes which value is being tested: The one that receives#should
call. This doesn't work withassert
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.
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
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
I'm already not a big fan of polluting Object
with should
and should_not
, adding more methods is only making that worse.
Right now we can do this:
That's fine but we could improve this situation.
In RSpec one can do:
This works by letting
be_empty
usemethod_missing
and creating a matcher on the fly. This can't work in Crystal, not because ofmethod_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 https://github.com/crystal-lang/crystal/pull/7500#issuecomment-559102417 are to be able to do something like this:
I pointed out that the readability of that can be improved a bit:
But when I went ahead to implement that I noticed in specs we have, for example:
Replacing that with
should_be
looks awkward:So I had that idea that we could have
assert
andrefute
, similar to other languages (but it's not the same as those other languages).Essentially, the above two examples would be:
Then we could also do:
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
andrefute
are very convenient methods to have, to avoid theshould be_true
boilerplate.The implementation is pretty straight-forward:
Thoughts? Any other names we could use?