Implement AssertAll

[andreoss]

Per #156

Extract interface from Assertion
Implement AssertAll & AssertWith
Introduce matchers Fails & Passes
Refactor tests

[codecov]

Codecov Report

Merging #248 (8b5a9a3) into master (97d6832) will increase coverage by 0.09%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master     #248      +/-   ##
============================================
+ Coverage     99.05%   99.15%   +0.09%     
- Complexity      165      184      +19     
============================================
  Files            36       40       +4     
  Lines           425      471      +46     
  Branches          8        8              
============================================
+ Hits            421      467      +46     
  Misses            4        4              

Impacted Files	Coverage Δ	Complexity Δ
...java/org/llorllale/cactoos/matchers/AssertAll.java	100.00% <100.00%> (ø)	8.00 <8.00> (?)
...ava/org/llorllale/cactoos/matchers/AssertWith.java	100.00% <100.00%> (ø)	3.00 <3.00> (?)
...java/org/llorllale/cactoos/matchers/Assertion.java	100.00% <100.00%> (ø)	4.00 <1.00> (+1.00)
...java/org/llorllale/cactoos/matchers/FailsWith.java	100.00% <100.00%> (ø)	5.00 <5.00> (?)
...in/java/org/llorllale/cactoos/matchers/Passes.java	100.00% <100.00%> (ø)	2.00 <2.00> (?)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 97d6832...8b5a9a3. Read the comment docs.

[victornoel]

@andreoss hey, thanks for the effort. I have to say I'm not sure I would like to introduce those changes to the codebase, there a few things that worries me, let me share them with you, maybe this could change my mind:

• if I understand well, this is based on the discussion of New constructors for Assertion #161: as discussed there, I think that it's best to compose matchers using AllOf or equivalent, and just have multiple methods to clarify the intent behind each of the tests:
  • even though the message of the assertion itself is sometimes a repetition of the method name, I would prefer to remove the message from the assertion in that case
• current implementation works well with existing test tooling such as JUnit: this change you propose is not going at all in the same direction
  • with this proposal, we loose the easy usage of existing JUnit extensions for tests (as a test class has everything crammed in one test method)

All in all, I think we are missing two important pieces here for this PR to be seriously considered:

• clear requirements that are not already solved by the current architecture and solved by this proposal
• to convert a few existing tests to it to see how it behave in practice

I hope it's not too much disappointing, but I'm open to discussion to improve cactoos-matchers :)

Also, I have been working on improving the architecture as I started to outline it in that comment: #239 (comment) (i.e., introducing our own interface to replace Matcher), maybe the requirements that your PR is trying to solve could be covered by the solution we found there?

[andreoss]

@victornoel

we loose the easy usage of existing JUnit extensions for tests (as a test class has everything crammed in one test method)

That's not the case, @TestFactory produces a separate case for each assertion.

[victornoel]

@0crat status

[0crat]

@0crat status (here)

@victornoel This is what I know about this job in C63314D6Z, as in §32:

• The job #248 is not in scope
• Job footprint (restricted area)

[andreoss]

@victornoel Some closing remarks

1. AllOf is a bad way to compose assertions, it causes all sorts of problems, unreadable messages, type inference, cluttered constructors.
   Also hamcrest project and Matcher implementations does not follow the same design principles as us
2. Assertion being a final class without an interface seems to be a bad design choice, prevents user from reusing existing assertions.
   I.e you have several objects with some desirable core behaviour, having one marcher for this behaviour means having one assertion (one test method).
   By using a test template approach you can compose core functionality assertions with exclusive functionality assertions producing as many tests methods (assertions) as possible.
3. Using a test object instead of test methods (Junit style) was proposed multiple times before, here it's just a proof of that concept which is made possible with extendable Assert type. As it still has Junit engine under the hood and I don't see that this approach breaks any integration with existing tools for Junit, (it works with mockito and spring Junit-5 annotations just as well). Overall the approach seems to be a step forward towards OO.

I still believe we should have multiple ways to write an assertion, instead of one Assertion class at the moment.

[0crat]

Job gh:llorllale/cactoos-matchers#248 is not assigned, can't get performer

[victornoel]

@andreoss thx for writing down a summary of your findings on that matter.

I completely agree with some of the problems you identify, mainly Matcher's being somewhat unfit to match cactoos' OO approach. On this, I started experimenting on introducing an alternative interface to Matcher that is simpler to implement and reason about. In that vision, Assertion would take such an object instead of Matcher. And we would provide an implementation of that interface taking Matcher's object for compatibility. I will create a ticket and/or a PR for discussing it, I hope you will share your feedbacks on it!

Also one problem with Matcher's implementations, e.g., AllOf that you cite, is that they were not meant to be used directly via their constructor but via static methods and so their constructor usage is sometimes cumbersome. This is just a technical limitation that can be solved via improved constructors. In the case of this new alternative interface it means we should be able to implement a nice to use AllOf-type of implementation.

Concerning Assertion being an interface, I don't see it as a problem actually, it's just that until now, I haven't seen any good reason to go down that path and thus I'm pushing making such a decision for as late as possible. It's a mix of laziness and carefulness let's say: I would prefer to have a clear use case for it and haven't seen it yet (except in this PR, which is a good start, but as-is as discussed, we won't be merging it). The next paragraph outline a way to do so :)

Concerning the tooling on executing tests (test object), while I like the approach, I don't think that this is what cactoos-matchers is about, at least for now: it should be an alternative to using Assertion, but this choice is for the user, not for us to make, so I don't think making it the only way of implementing tests is correct. So, to open the door for going that way, I would propose to create a new ticket for a proposal of design to add to cactoos-matchers on top of its existing components, if you would like to. This could rely on the fact Assertion must be an interface of course.