Replace expectations in spec with power asserts

[RX14]

After using groovy for a while, and loving the power assert built into the langauge, and also using the wonderful power_assert.cr by @rosylilly, I wanted to propose adding power assert to crystal's stdlib. I believe that powerful assert should replace the expectations dsl in spec because it is cleaner, and can show you more information, which is helpful to debugging.

Powerful asserts consist of assert before an expression. When the expression returns false at runtime, an exception is generated containing a deconstruction of the statement with debug information on every part. For example:

foo = "foo"
assert foo.index('o') == 3

foo.index('o') == 3
|   |          |
|   |          false : Bool
|   1 : Int32
"foo" : String

Compare this too the default spec expectations:

foo = "foo"
foo.index('o').should eq(3)

expected: 3
     got: 1

In the power assert example you get much more context to the spec failure than you do in the expectations example, by slowing the line of code that failed and the values at various parts of the expression. This deconstruction grants you visibility into exactly what went wrong, which lets you debug faster.

The syntax is also clearer than the expectation syntax, because it's a normal crystal expression instead of having to learn a new DSL to express yourself with. There are less brackets, it's cleaner. For example, the code samples above. Finding an == is much easier visually than finding .should eq( in an expression consisting of method calls.

All of the expectation methods are easilly translatable to normal crystal expressions, except #close, and in this case I would argue that Float#close? should exist to enable fuzzy matching outside of specs.

@BlaXpirit has converted a whole file of stdlib specs to assertion syntax, in a gist here. There are a few warts in power_assert.cr, mainly to do with lacking proper resugaring or not showing types, visible here. These can be fixed before merging in.

It should be possible to implement a crystal formatter to convert expectation syntax into assert syntax quite easilly. Expectations could be deprecated, and power assertions and the formatter changes could be released in 0.19.0, followed by removing expectations in 0.20.0.

[asterite]

Compare this:

require "spec"

describe "something" do
  it "does something" do
    {% for i in 0..1_000 %}
      1.should eq(2)
    {% end %}
  end
end

To this:

require "spec"
require "power_assert"

describe "something" do
  it "does something" do
    {% for i in 0..1_000 %}
      assert 1 == 2
    {% end %}
  end
end

On my machine, the first takes less than a second to compile, the second takes 24 seconds. So unless power_assert can improve that time I don't think this will happen.

[asterite]

I also think power assert isn't extensible. All the rules must be defined in the assert macro. spec is extensible, you can define custom matchers. And the error messages are pretty good in my opinion (and could be improved further, if we had a diff module in the standard library).

[RX14]

Well, there's not much power_assert can do about macros being slow, it's assert macro is pretty simple after all. I think we should be looking at the benefit to the developer, then look at how to optimise it (or macros) if we decide we want it.

Regarding extensibility, power_assert effectively makes matchers simply runtime calls on existing objects. If you want a close matcher for floats, you should define Float#close? by reopening the class. The number of methods that return booleans on every aray, string, etc. is already much higher then the number of matchers. This way matchers don't have to be spec specific, they are avilable to be used at runtime by any code.

While the error messages for spec expectations are ok, i think it's clear that power assert error messages are much nicer, and show much more context.

[asterite]

I can see that power assert generates, for assert 1 == 2, this:

``` cr __temp_20 = 1 == 2 unless __temp_20 __temp_21 = get_ast(1 == 2) __temp_22 = __temp_21.breakdowns __temp_23 = String.build do |io| io << " " * PowerAssert.config.global_indent __temp_21.to_s(io) io << "\n" __temp_22.to_s(io) end raise AssertionFailed.new(__temp_23, "/Users/asterite-manas/Projects/crystal/foo.cr", 371) end __temp_20 __temp_24 = get_ast(1) __temp_25 = [] of PowerAssert::Node __temp_25.push(get_ast(2)) __temp_26 = [] of PowerAssert::NamedArg __temp_27 = "" PowerAssert::MethodCall.new( "==", (1 == 2).inspect, __temp_24, __temp_25, __temp_26, __temp_27 ) PowerAssert::Node.new("1", (1).inspect) PowerAssert::Node.new("2", (2).inspect) ```

So that's a lot of code to type for each assertion. Plus it's allocating a lot of memory (arrays, nodes) for a simple comparison.

If all of this can be improved then we can consider adding it to the standard library, but I can't really see how, given that it prints the value of every subexpression in an assertion.

I also don't think it's a bad idea if power_assert is kept as a shard that you can optionally use in your projects, in case where it's small and this slowdowns aren't a big deal.

[RX14]

If we created a serialised JSON version of the AST, which can be efficiently generated by the crystal compiler, then allowed passing that to a crystal program which would generate an efficient block of code, that could work.

Putting the codegen in the compiler could also work, but i'm sure you wouldn't want that.

I think it must be possible to achieve, we just need to avoid as much codegen as we can for each assertion.

I think the optimal codegen for assert 1 == 2would be as so:

__temp_1 = 1
__temp_2 = 2
__temp_3 = __temp_1 == __temp_2

unless __temp_3
  message = <<-MSG
  1 == 2
  | |  |
  | |  #{__temp_2.inspect} : #{typeof(__temp_2)}
  | #{__temp_3.inspect} : #{typeof(__temp_3)}
  #{__temp_1.inspect} : #{typeof(__temp_1)}
  MSG
  raise AssertionFailed.new(message, "foo.cr", 12)
end

Which seems like it would be much much easier on the codegen and typing because of the lack of using and instantiating classes. I'm sure the compiler could generate this amount of code for each assert pretty quickly, it's just impossible to sanely do the message generation in a macro so you have to bail out to "real crystal".

I might have a go at getting this to work, and be optimised. I think if groovy (which kind of does compile to bytecode at least) can get this working, then crystal can.

[oprypin]

@asterite, I see that it's problematic to implement this in userspace. But would you consider having such an assert statement added to the compiler?

I have been working on a proof of concept to implement this using technology from the compiler, but I run it as an external process for now, to see if this is viable before trying to put the code (hopefully mostly unchanged) into the compiler.

I especially like this revision. The code is simple and fast, but provides most of the same functionality as power_assert. Advanced features and corner cases would probably make it uglier though.

Too bad it can't really be benchmarked in this state.

---

@asterite, you said:

[...] I can't really see how, given that it prints the value of every subexpression in an assertion

Actually, seeing the details about the subexpressions is a big advantage, not sure why you see it as negative. Without recursive descent this would be much, much simpler to implement. Not as nice but still much nicer than with the DSL.

[oprypin]

Forget about the prototype, I have implemented this in the compiler.

https://github.com/crystal-lang/crystal/compare/master...BlaXpirit:feature/assert

Benchmark shows good results - only 15%-20% slowdown (whereas _powerassert caused ×100 slowdown)

Example outputs

[bcardiff]

I like @BlaXpirit approach. It would be great if the compiler could be extended to support this kind of transformations in an efficient way and the customization are subject to each project, but we are not there.

• What happens with big structures that span across multiple lines? Do they get trimmed by ellipsis or the break?
• A disadvantage of this asserts is that they are too visual and does not have a semantic representation. For sure it has more information than a #should, but the user is able to customize the message for better matching (like with strings or arrays, figuring out a diff). Customizing a should/matcher is far more easy than changing the compiler. At the end of the day this is because power assert is a best effort to get the semantics of a boolean expression, whether a should models the expectation itself.

[oprypin]

@bcardiff, you make a good point about customization that I didn't realize before. Indeed, things like "figuring out a diff" will become unavailable to the user. But I think the fact that it provides so much out of the box beats this disadvantage (this is almost never used anyway).

What happens with big structures that span across multiple lines?

Currently? It's just a horrible mess.

For now I don't know if this has a chance to be included in the compiler. If so, we can figure out the details along the way.

[ozra]

Just want to say that I like the idea of making assertion capabilities native to the language and/or compiler.

[RX14]

Honestly, I just wish that the macro system was powerfull enough that this didn't need to be in the compiler. The fact that the macro implementation was 100x slower is an indication of how inefficient it currently is to get compile-time infomation to the runtime.

[asterite]

@BlaXpirit I find it really amazing/nice that you was able to add this to the compiler, basically touching compiler code. Awesome!

That said, I don't think the current spec behaviour is bad, and, as I said before, it's extensible. For example we could make comparing strings show a diff. In case of a failure you'll need to debug the code, and I doubt that only by looking at sub-expressions values you'll figure out why the spec is failing, and how to fix it.

[RX14]

I think that having compiler plugins, similar to rust, so this functionality can be tested external to the compiler, would be extremely helpful. I truly believe that the benefits of powerful assertions heavily outweigh the downsides of switching from expectations, but if this won't get merged in it's current state, then the only way this will ever be tested to maturity will be as a compiler plugin.

[oprypin]

I really can't come up with any useful extension other than showing diffs. In that case, what's the problem with defining a diff method for objects and using it (if available) for top-level expressions in assert?

[oprypin]

Let's look at it the other way around. Imagine the assertions are in the language. Would you really want to replace them with the complex DSL that makes tests unreadable and scares new users, just for the potential extensibility alone? Would this DSL even exist if it wasn't a forced measure in Ruby?

Once we accept that assertions are better out of the box (@asterite, I still don't see why you oppose the detailed breakdowns), we are free to think about ways to make them better and extensible if deemed necessary.

The latest idea I had was the ability to supply a 2nd expression which shows additional information in case of failure. Simplest example (although it is not necessary because the information is clear by default):

assert a.is_a? Int32, "expected #{a} to be Int32"

This would show the message in addition to the normal output.

Or for the diff:

def diff(a : Array, b : Array)
    "values differ at index 5"
end

assert a == expected, diff(a, expected)

Yes, maybe the code is longer, but it is so much simpler to work with this than expanding the expectations DSL (I have never seen it done anyway).

And the ability to define custom macros that raise custom AssertionFaileds doesn't go anywhere (though relaying the file and position could use some work under my implementation). assert_raises can stay as is, and users are free to define macros that do diffs or whatever (even rescuing AssertionFailed and extending its message works).

---

So, instead of using the implementation deficiencies as arguments against the asserts, maybe we can embrace just the idea and think about how to make it most useful. Then we can make an objective final decision and move on, one way or the other.

[ozra]

The notion of plugins for analytics and such sound interesting - but a good basic powerful generically usable assert built in sounds good too. You can never make testing too easy!

[asterite]

@BlaXpirit Could you send a PR with this so I can easily test it locally?

As with always, I'm never fully against something. It just worries me adding a keyword for this, hardcoding this in the compiler, and making things a bit slower. Making a language and all of its standard library is a huge task where all of the time you have to make small and big decisions, and they have to be coherent, so it's very difficult and challenging to do so. I don't expect this to get to 0.19.0, but we can continue discussing and improving this until we decide to merge it or do something else.

[oprypin]

@asterite Actually, my latest benchmark (of specs in the compiler itself) showed almost no slowdown.

The branch is there available to test. PR coming soon.

[asterite]

@BlaXpirit Ah, then don't worry about a PR yet. I'll checkout that and try it. I must say that it does look more unified, specially for things like foo.should contain(...) vs. assert foo.includes?(...)

[ozra]

On the keyword part, I had the exact same reaction: I think it should stand out a bit more for such "magic". Perhaps it should be "pragma-like" in syntactic appearance?

[oprypin]

There is not much magic that affects the user. It's really just assert expression → expression || raise AssertionFailed(info)

[ysbaddaden]

You want to make assert a keyword?! Do you mean to kill my minitest port? :sob:

I don't like the output of power_assert. I consider it noisy and hard to read (sorry, I'm weird). If it had stayed purely in spec in the stdlib, I wouldn't care and would have kept my mouth shut. But if it comes down to the corelib (FailedAssertion) and more importantly down to the compiler (assert keyword), harcoding the behavior... there is no way I can be happy with this.

[ozra]

@BlaXpirit - I was just concerned with tying up the identifier assert, like @ysbaddaden touched upon.

[oprypin]

@ysbaddaden well yes, minitest embodies everything that we criticize here and takes it to the next level...

If you don't like the current implementation, that's fine, and you're free to suggest a different look for the output. I've previously said, the output does not even have to be recursive, could be limited to the topmost expressions as it currently is, but the change still gets rid of DSLs.

Having AssertionFailed as part of the core standard library is a change needed regardless of this feature because it makes it possible to use multiple test frameworks at once (mainly I mean frameworks that can optionally cooperate with standard library's spec).

assert being a keyword is a forced measure because userland access to the AST is not fast and powerful enough. Not keeping it to spec could also be seen as a nice feature. The assert keyword is nice for debugging and specifying things like "this must never happen"... the assert lines could even be stripped from executables in release mode, like C and Python do (and yes, assert is a keyword in these and other languages).

[ozra]

Could be a nice feature to allow some options passable to the compiler for different output styles / levels? That asserts are stripped out in release mode is a given in my opinion! I have had (ok, ok, in javascript) assert methods implemented on objects for integrity validations (removed in dist-build), so that's why I immediately felt "hey, we need assert as an identifier". But realistically, perhaps it's not that much of an issue. (Except for some perople having to make a few changes in existing code, which isn't a biggie in Crystal < 1.0)

[asterite]

I'll try to implement a simplified assert macro with just a few cases and see how it goes. I don't think this needs compiler support, at least if we drop support for nested expressions (which against, some of us don't think it's very useful)

[RX14]

@asterite Nested expression output is very useful when dealing with collections. Very often your assertions reduce a collection down to a single value or boolean, which is useless for debugging. Being able to see the collection you used in the assert statement is super useful.

Keep in mind, that I don't want to make assert a keyword, or place it in the compiler either. But the slowness of macros, especially recursive ones and the lack of compiler plugins rather makes it neccesary for large projects.

In regard to the name conflict, could we not use a @[Primitive] style fake-method to place assert into the spec library?

@ysbaddaden I think that the proliferation of spec libraries already proves that there will never be consensus on this issue, FailedAssertion is an attempt to make spec libraries compatible with assertion tools. If every assertion method uses FailedAssertion and every spec library special-cases FailedAssertion to be an assertion failure not an error failure we can let users mix and match.

[refi64]

well yes, minitest embodies everything that we criticize here and takes it to the next level...

Ouch.

[asterite]

This is what I came up with

``` cr require "spec" macro assert(exp, file = __FILE__, line = __LINE__) {% if exp.is_a?(Call) && (obj = exp.receiver) %} {% if exp.name == "==" %} ({{exp.receiver}}).should eq({{exp.args[0]}}), {{file}}, {{line}} {% elsif exp.name == ">" || exp.name == ">=" || exp.name == "<" || exp.name == "<=" %} ({{exp.receiver}}).should (be {{exp.name.id}} ({{exp.args[0]}})), {{file}}, {{line}} {% else %} %obj = {{exp.receiver}} %args = Tuple.new({{*exp.args}}) %res = %obj.{{exp.name}}(*%args) unless %res fail "expected #{ %obj.inspect }.{{exp.name}}(*#{ %args }) to be truthy but was #{ %res }", {{file}}, {{line}} end {% end %} {% else %} ({{exp}}).should be_truthy, {{file}}, {{line}} {% end %} end describe Int32 do it "#+" do assert 2 + 2 == 5 end it "#even" do assert 3.even? end it "#>" do assert 3 > 4 end it "includes" do elems = [1, 2, 3] target = 4 assert elems.includes?(target) end end ```

You can see it here: https://play.crystal-lang.org/#/r/18ao

The output messages could be improved a bit, but not much. For example:

expected [1, 2, 3].includes?(4) to be truthy but was false

it basically spits out the expression back and I have to reconstruct its meaning. Add a few vertical lines to that, with values for subexpressions, and it becomes much harder to read and understand (again, this is my opinion). With the current spec, using elems.should contain(target), I get:

     Failure/Error: elems.should contain(target)

       expected:   [1, 2, 3]
       to include: 4

So for me the output is more human and easier to understand. Not only that, but there wasn't a need to do it with macros, and, as I keep saying, it's extensible.

This is why I'm not sure I like another approach to this. The current one is pretty good in my opinion, fast, and extensible.

I understand that complex macros could be a bit slow. I'd like to see that as a virtue, so macros aren't abused and one focuses more on runtime code instead of compile-time code (though this doesn't mean we'll make our best to make macros faster and faster over time)

As a side note, I think minitest is a very good alternative to spec, letting you use regular OOP solutions in tests, probably using setup/teardown, etc. I think spec/minitest or other frameworks really depend on the use case. I personally prefer spec for libraries because usually there's not much setup/teardown (for example for a library providing some useful methods). But in bigger projects where DB is involved and other things, minitest might be better. It's good that the language doesn't force you to use only one approach. Hardcoding assert in the language will make this a bit harder, as @ysbaddaden says.

[ysbaddaden]

There are many assertions, expectations and test/spec frameworks. Nobody agrees on a common solution (as is exacerbated in this thread). I don't want to force anything into Crystal, especially at the expense of alternatives, when all these solutions already coexist as shards.

@asterite more importantly than setup, I abuse helper/assertion methods that are local to a test class, or scoped to Minitest::Test (thus not leaking to Object).

[RX14]

My number one problem with the current expectations is that the DSL is really ugly. Powerful assertions are my favoured way of fixing it, and I really do think it has benefits which outweigh the costs. Unfortunately it's hard to implement without being able to run arbitrary code at compile time with access to the AST. Also the power assert error messages contain a lot more information than the spec error messages, which can be a bit cluttered.

Still, I believe that the expectations DSL is ugly, and I think many will agree. The clear solution to me is to allow spec assertions to be written using the powerful, clean DSL we already have: normal expressions using normal methods on normal objects. Powerful assertions is simply an implementation of this solution which retains the ability to see subexpressions before they are reduced down to a single boolean value.

I'm not entirely sure how to implement this solution without the performance and (possible) readability concerns of powerful assertions, but I think that if it can be done, it should.

[oprypin]

I agree with @RX14. I'm ready for any compromise to avoid DSLs and use the language naturally.

If we were to use a simpler assert macro, that doesn't mean it can't be extensible. How about this:

module Enumerable(T)
  assert_info includes?(obj) do
    "Expected #{inspect} to contain #{obj.inspect}"
  end
end

arry = [1, 2, 3]
assert arry.includes? 4

Expected [1, 2, 3] to contain 4

This would use the following additions to standard library (can be easily isolated to spec):

module Spec
  @@assert_info : String?

  def self.assert_info=(obj)
    @@assert_info = obj
  end
  def self.assert_info
    @@assert_info
  end
end

macro assert_info(meth, &block)
  def {{meth}}
    %res = previous_def
    Spec.assert_info = %res ? nil : {{yield}}
    %res
  end
end

macro assert(exp)
  Spec.assert_info = nil

  unless {{exp}}
    if (%assert_info = Spec.assert_info)
      raise %assert_info
    else
      raise "Expected #{ {{exp.stringify}} } to be truthy"
      # Add more special cases as seen in @asterite's macro above
      # The fallback cases can be the same as what Spec expectations currently cover
    end
  end
end

Yes, this is a bit hacky, but it should be fine because it's isolated to specs.

(Note that this code fails on Crystal 0.18 for some reason)

[olbat]

I'm ready for any compromise to avoid DSLs and use the language naturally.

I have the same feeling about DSLs I find the code more "readable" without them.

@ysbaddaden is right, it seems to be hard to find a solution that will suit everyone ...

I just keep thinking that it'll be really nice to be able to use spec without the DSL, to write tests using the core lib that seems more "readable"/natural to me. Powerful assertions definitely seems to be a good solution.

[ozra]

How about just making a program that includes the compiler lib, add power-assert functionality, and have a power-asserts-checker program. It's a shard. It's fast. It's not in the official compiler. Of course this means a dev-time dependency on that for any project using the power-asserts. Or an alternative slower macro-based version in addition. Just a thought of the top of my head.

[oprypin]

@ozra, that's a good idea, I've already considered it, but rejected it for now. Mainly because of the complexity; it might even require its own testing framework. If someone wants to work on this with me, that would be nice.

For now I'm settling for a simple assert macro that wraps standard library's expectations, much like asterite showed above. Though I'm running into problems even with such a simple macro. For example, Not and IsA nodes have no methods at all in macro world......

[ozra]

PR lacking methods to the macro interpreter? :-)

[miketheman]

codetriage

In an attempt to bring resolution to this issue, I've read through and tried to figure out what, if any, open actions remain to resolve this one way or another.

Is this still an open discussion? Is PowerAsserts something that we are still considering including into the default, or should it be a standalone shard, like minitest or spec2?

[akzhan]

I suppose that compiler functionality may be added to Crystal if required.

All other functionality should be separated as shard.

[faultyserver]

I agree that the full power_assert should be kept as a shard, not made part of the stdlib. However, I think adding the basic assert that oprypin most recently proposed is a good compromise. It allows those that don't want to use the expectation/should syntax to assert arbitrary expressions and give custom error messages on failure. For those wanting more in-depth analysis and output, the shard would still be available.

My take (all of these are purely opinions):

• Similar to ysbaddaden, I find the output of power_assert hard to read and mostly unnecessary. A good example can be found here.

• Relating to the above, I think assert, in general, makes it too easy to write overly complex single assertions:

  # This seems like a bad test. It makes assumptions about everything beyond
  # `obj` and relies on the verbose test output for insight into any failures.
  assert obj["foo"][1]["bar"][1].as_i == 2
  # I think it should be re-written as multiple assertions/expectations:
  obj["foo"].should              be_a(JSON::Any)
  obj["foo"][1].should           be_a(JSON::Any)
  obj["foo"][1]["bar"].should    be_a(JSON::Any)
  obj["foo"][1]["bar"][1].should be_a(JSON::Any)
  obj["foo"][1]["bar"][1].as_i.should eq(2)

  The second is more verbose, but I think it makes for a more robust, complete test and better follows Single Responsibility. (granted, both are possible each way. I just think assert makes the former seem too correct/encouraged)

• There's no requirement to use the should DSL in tests. Just raise and the test will fail. Other shards can help with output formatting, etc.

• As others have mentioned, consensus is essentially impossible, and I don't think it's the onus of the language to set any particular precedent (Ruby had Test::Unit forever, but practically no one used it. minitest, rspec and the like took over and that was fine).

Miscellaneous:

• Elixir has a very good assert implementation that could serve as a guide for improving the proposed, simple assert given above.

• I think assert would actually be rather useful (potentially more useful) outside of specs as well. C/C++ has assert and static_assert for doing run-time/compile-time assertions in application code to aid development. This SO comment gives a pretty good rationale for using them in application code.