Skip to content
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

Extensibility of Assertion #156

Open
andreoss opened this issue Jun 20, 2020 · 6 comments
Open

Extensibility of Assertion #156

andreoss opened this issue Jun 20, 2020 · 6 comments

Comments

@andreoss
Copy link
Contributor

andreoss commented Jun 20, 2020

Assertion2 is a final class without an interface.
It blocks the user from

  • creating custom assertions
  • decorating Assertion with additional logic
    And also violates "He Works by Contracts"1 principle of EO.

Examples of use cases:

  • conditional assertions, covers the cases of @EnabledIf from Junit 5
  • soft assertions, i.e not fail directly but mark test as failed in some kind of storage for later evaluation
  • repeated and grouped assertions, covers Assertions.assertAll for Junit 5

Proposal:

  • Extract an interface named Affirmation (after the method)
  • Discuss which additional implementations which should be brought
@0crat
Copy link
Collaborator

0crat commented Jun 20, 2020

@paulodamaso/z please, pay attention to this issue

@andreoss andreoss changed the title An interface of Assertion Extensibility of Assertion Jun 20, 2020
@andreoss
Copy link
Contributor Author

@paulodamaso @llorllale Ready to implement if green-lighted.

@llorllale
Copy link
Owner

@paulodamaso I think the first two use cases make sense:

  • conditional assertions, covers the cases of @EnabledIf from Junit 5
  • soft assertions, i.e not fail directly but mark test as failed in some kind of storage for later evaluation

WDYT?

@paulodamaso
Copy link
Contributor

@victornoel WDYT?

@victornoel
Copy link
Collaborator

@andreoss @llorllale @paulodamaso sorry for the late answer on this :)

Basically, I think it's a good idea to introduce an interface yes. At first I was thinking like @llorllale with his comment about assertAll being the same as AllOf (see #161 (comment)), but I feel like that if we want to have multiple conditional and non-conditional assertions we can't rely on AllOf Matcher for that.

At the same time, I think that if we need multiple assertions, then we should write multiple test methods. Why would we want to group them in one place?

So, I propose to start with a simpler ambition:

  • let's introduce the SoftAssertion class
  • whose constructor takes multiples Assertion object
  • it's affirm method should catch all Exception (or only RuntimeException actually?) and AssertionError for each so that they can all be run
  • it should at the end throw a AssertionError with multiple suppressed causes
  • of course we need to introduce the Affirmation interface that is documented about what it throws

So, @andreoss @llorllale @paulodamaso @fabriciofx (bringing you in the discussion ;), each of those item of the proposal are open to discussion, what do you think?

@andreoss
Copy link
Contributor Author

@victornoel

Not sure if directly related to this discussion, but the problem I see with new Assertion(..)affirm() is that one has to call
affirm() method explicitly (which is an easy thing to forget) in each method imperatively.
A more proper way would be to return Assertion. i.e

@TestFactory
Assertion foo() { return new Assertion(...); } 

In order for Assertion to be detectable by Jupiter's @TestFactory it should be implemented like that

public interface Assertion extends Iterable<DynamicNode>  { 
    ... 

    @Override
    default Iterator<DynamicNode> iterator() { ... } 
} 

This sequentially leads to a shorter boiler-plate free form of writing tests (proposed in https://github.com/pragmatic-objects/oo-tests):

final class FooTest extends TestEnvelope { 
    FooTest() {
        super(
            new AssertThat<>(1 + 1, new Is(2)),
            new AssertThat<>(2 + 2, new Is(4))
        );
    }
}

On naming, it seems now that Affirmation is a poor choice, and it would be better to stick with Assertion and keep Assert... as a common prefix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants