-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Replace expectations in spec with power asserts #3095
Comments
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. |
I also think power assert isn't extensible. All the rules must be defined in the |
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 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. |
I can see that power assert generates, for __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. |
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 __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. |
@asterite, I see that it's problematic to implement this in userspace. But would you consider having such an 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 Too bad it can't really be benchmarked in this state. @asterite, you said:
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. |
Forget about the prototype, I have implemented this in the compiler. master...BlaXpirit:feature/assert Benchmark shows good results - only 15%-20% slowdown (whereas power_assert caused ×100 slowdown) |
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.
|
@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).
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. |
Just want to say that I like the idea of making assertion capabilities native to the language and/or compiler. |
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. |
@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. |
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. |
I really can't come up with any useful extension other than showing diffs. In that case, what's the problem with defining a |
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 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. |
The notion of plugins for analytics and such sound interesting - but a good basic powerful generically usable assert built in sounds good too. |
@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. |
@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. |
@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 |
On the keyword part, I had the exact same reaction: I think it should stand out a bit more for such "magic". |
There is not much magic that affects the user. It's really just |
You want to make I don't like the output of |
@BlaXpirit - I was just concerned with tying up the identifier |
@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
|
Could be a nice feature to allow some options passable to the compiler for different output styles / levels? |
I'll try to implement a simplified |
@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 @ysbaddaden I think that the proliferation of spec libraries already proves that there will never be consensus on this issue, |
Ouch. |
|
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 |
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. |
I agree with @RX14. 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
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) |
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 |
How about just making a program that includes the compiler lib, add power-assert functionality, and have a |
@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, |
PR lacking methods to the macro interpreter? :-) |
#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? |
I suppose that compiler functionality may be added to Crystal if required. All other functionality should be separated as shard. |
I agree that the full My take (all of these are purely opinions):
# 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
Miscellaneous:
|
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:Compare this too the default spec expectations:
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 thatFloat#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.
The text was updated successfully, but these errors were encountered: