crystal-lang / crystal

The Crystal Programming Language
https://crystal-lang.org
Apache License 2.0
19.48k stars 1.62k forks source link

Introduce `Spec::Expectation` module #12862

Open Blacksmoke16 opened 1 year ago

Blacksmoke16 commented 1 year ago

The spec module works by passing an Expectation to #should or #should_not as the main way of writing assertions. However there is not an actual interface representing what an Expectation actually is. E.g. all the method examples in https://crystal-lang.org/api/1.6.2/Spec/Expectations.html mention it, but is kinda left as this obscure type that exists, but isn't really defined.

I've recently been working on a new Athena feature in regards to how it handles integration testing. During this process it made sense have create some custom expectation types as a means to represent some domain assertions.

Assuming defining custom expectation types is something we want to encourage/allow, we should probably introduced an actual Spec::Expectation module, such that it can be documented. E.g. something like:

module Spec::Expectation
  abstract def match(actual_value : _) : Bool
  abstract def failure_message(actual_value : _) : String
  abstract def negative_failure_message(actual_value : _) : String
end

Also given all of these are currently :nodoc:, it would probably be also worth thinking about the API of it more before introducing the interface.

Some similar types from other langs:

Blacksmoke16 commented 1 year ago

Given this issue is getting some πŸ‘s, a topic we should also discuss related to the Expectation API is how to handle like logical expectations. I.e. if people are allowed/encouraged to create custom expectations, it's not out of the question to want to do logical operations with them such as AND, OR, XOR, NOT and such.

Currently you have to do this via multiple single expectations:

# Value greater than 0, and equal to 10
value = 7
value.should be > 0
value.should eq 10

But of course the downside is it would fail on the first expectation and if you change the value, it could then fail on the other. As opposed a nicer message that showed your intent. A more concrete use case would be asserting an HTTP response is a redirect, with a specific location and status; which would be 3 different expectations AND'd together.

Sorry for the PHP example πŸ˜…, but they handle it like:

self::assertThat(
    -1,
    LogicalAnd::fromConstraints(new GreaterThan(0), new IsIdentical(10))
);

Which would produce an error like Failed asserting that -1 is greater than 0 and is identical to 10. Similarly negation is handled via like new LogicalNot(new IsIdentical(10)) with a failure message of Failed asserting that 10 is not identical to 10..

Would be doable to have a similar API if thats the route we want to go. Main benefit would be making it so you only have to really define the positive message and not essentially duplicate everything twice with only 1 extra word or something.

Of course this could be made backwards compatible with the built in expectation methods and such, especially given all the current expectation types are not part of the public API anyway.

HertzDevil commented 1 year ago

There are actually two custom expectations in this repository already:

https://github.com/crystal-lang/crystal/blob/75a3a47013f47f5773e00a2e7768bbce96547a95/spec/manual/string_normalize_spec.cr#L7-L28

https://github.com/crystal-lang/crystal/blob/75a3a47013f47f5773e00a2e7768bbce96547a95/spec/compiler/semantic/metaclass_spec.cr#L3-L53

IMO composing arbitrary failure messages makes sense only for the simplest scenarios. Another downside of composing expectations is that spec failures no longer communicate the exact locations of the failures; this sounds like the job of a custom assertion, rather than a custom expectation. Finally I don't really think most specs would make use of the OR and XOR combinators, as the resulting expectations are strictly less specific.

Sija commented 1 year ago

@Blacksmoke16 I'd rather go with chained .should[_not].

PS. every time you post PHP code, god kills a kitten. Please, save the kittens πŸ™

Blacksmoke16 commented 1 year ago

The PHP was simply an example of how they decouple the negation/logical operators from the expectation itself. The API could be crystalized, say something like -1.should and(eq(10), be > 0).

expectations is that spec failures no longer communicate the exact locations of the failures

You mean that you wouldn't know which part of the expectation failed if there was two expectations AND together? Otherwise the location of the failure would still be whatever #should or #should_not method was called.

I looked into it more and their LogicalAnd and such are implemented in a way where they themselves are also an Expectation. All of this could really be implemented in a shard in that case.

I don't really have strong feelings about it. Only bringing this up as if we want to merge failure_message and negated_failure_message into one, letting something else handle negation, we should do that now before introducing the interface.

HertzDevil commented 1 year ago

if we want to merge failure_message and negated_failure_message into one

We shouldn't. Their messages agree only for the simplest cases.

Another thing is that whereas should(be_a Int32).should(be < 10) performs a type filter such that the second should's receiver and the whole expression are both Int32s, should and(be_a(Int32), be < 10) doesn't do it easily. It also follows that AND is not commutative because the be_a here must go first. So I would exercise extra caution when composing expectations in this manner.