Create assertions for Pioneer (#232)

[Michael1993]

Created assertions for Pioneers ExecutionResults class with a (hopefully) intuitive fluent API.

Closes #232

---

I hereby agree to the terms of the JUnit Pioneer Contributor License Agreement.

[nipafx]

I really like what the refactored tests look like, this was exactly what I was hoping for: very clean and in the domain-specific language.

Some ideas on the implementation:

• since you have several classes, let's create a new package org.junitpioneer.testkit.assert and put'em there as their own top-level classes
• I like the fluent API with several interfaces and think it would be more clear cut if all methods that belong to the API came from an interface
• then PioneerAssertions::assertThat can return an interface - more strictly, none of the methods should ever return a concrete class

You're definitely on the right path, so keep going. 👍

[aepfli]

Cool today i learned how to write assertJ containers, and this is nice. Good Job!

i am still not sure about the naming of different methods, and i would try to check assertj and find examples how they are used. I already do think that this fluent api is a big improvement. and i like this changes a lot. a little fine tuning with the naming, and maybe some generalized methods would be cool

[aepfli]

i am not sure if the and is really needed, and it is not really common in assertJ. i think hasException should be fine, or maybe withException.

not 100% sure but i think we should align this with https://joel-costigliola.github.io/assertj/assertj-core-features-highlight.html#exception-assertion . so it is really concise and easy to understand, when you know assertj already

[Michael1993]

My reasoning behind andHasException is that you can only get there if you already called a method that returns with the FailureAssert interface.

So it always looks like this:

assertThat(results).hasXY(...).andHasException();

In a purely grammatical context the and makes sense(?).

[Bukama]

To put an example to the discussion:

assertThat(results).hasSingleFailedTest().andHasException().withMessageContaining("ne...")

My first thought about naming was: We always compare results which is plural, so I asked myself if the methods should align with plural too. But I don't think so, because the default AssertJ lib claims only a single result, independent if its a single value or a list or whatever. So we should go for singular method names too.

What I really like at @Michael1993 names is that he always has something in mind when choosing the names. Was the same at the publish condition of the @ReportEntry extension. And I can totally understand his thoughts here to. On the other hand I can see @aepfli point to align with the default AssertJ which uses withException" and I could imagine that this helps others people to use the API. While I see the use of andfrom grammatical context, I don't think we need it as there is always the.` of the fluent API which says exactly this.

[aepfli]

i might suggest to have one basic method like hasNumberOfFailedTests(int number) and than just use that within this method. as i think this assert might be usable for other cases in the future too.

[Bukama]

I would like to keep the single methods and offer additional methods where you can pass the number, because we have plenty of tests where we expect exactly one. So why force the writer to pass magic numbers and make it less readable in those cases? Don't get me wrong. We need the basic methods for dynamic parameters, but this PR shows that the single ones makes sense too.

[aepfli]

i never said to skip that method, i said we should extract its functionality into an generic one, which we use within this method

[Bukama]

I don't want to add anything more and think @nicolaiparlog thoughts about a "more beautiful" implementations are valid (That's why I chose "Request changes"). I also comment on the two naming things, but I'm totally fine with the current names too! They are already very good and we discuss micro optimizing here I think.

Overall thank you very much for the PR and showing us all how simple it is to provide own assertions in an AssertJ style! Really like to read the improved tests! 👍

[Bukama]

I leave commenting about the implementation details to @nicolaiparlog. Things missing for me, when he says that it's okay:

• Javadocs on public methods and Interfaces
• Documentation with examples for website (navigation and adoc-file)

[Michael1993]

I was under the impression that this is test code and does not get bundled in JUnit Pioneer - so it is for internal use only. I agree that adding Javadoc is needed but why add examples to something users can not access?

[aepfli]

i find this really cool, cant stop saying that. There are just some questions i want to ask or suggestions i would love to propose.

assertThat

Currently we are limited as we only have our asserts when using assertThat i can imagine that adding another assertThat from assertJ core as tidious and not really intuitive. therefore i would suggest to also extend the basic assertion class (if that is possible) like descibed in https://joel-costigliola.github.io/assertj/assertj-core-custom-assertions.html

(optional) same would be cool for softassertions, if we might need them someday

exception assertions

there is already a quiet powerful exception assert from assertJ and i highly recommend to use that one, instead of our own. it is intuitive if we behave the same way with those, as assertj is doing.

but again, i like this a lot and i learned so much :)

[Bukama]

I was under the impression that this is test code and does not get bundled in JUnit Pioneer - so it is for internal use only. I agree that adding Javadoc is needed but why add examples to something users can not access?

For contributors. Or as @nicolaiparlog would say: `You. Me. And JUnit Pioneer". I like when tools are documented, so everybody knows about them and knows how to use them.

[aepfli]

i think this looks really cool now. and it looks good to me. Also might be quiet a while that i looked at code here, but i think this is ready to get merged

[Bukama]

I think we can turn this into a real PR, can we @Michael1993 ?

[Bukama]

Thank you for turning it into a PR. Even that @aepfli and I had already approved I leave the PR open and assign it to @nicolaiparlog , because I'm nearly 100% sure that the wants to merge it on stream. We already had a look on this PR on stream and it's quite a real "core feature" of Pioneer, so perfect for stream.

So please don't be sad that you most probably won't get any update until then (I expect the next Pioneer stream to be in early June)

[Bukama]

The build breaks because of compiling errors:

> Task :compileTestJava FAILED
/home/runner/work/junit-pioneer/junit-pioneer/src/test/java/org/junitpioneer/testkit/assertion/PioneerAssert.java:26: error: cannot find symbol
	public static class EntryPoint extends Assertions {
	                                       ^
  symbol:   class Assertions
  location: class PioneerAssert
/home/runner/work/junit-pioneer/junit-pioneer/src/test/java/org/junitpioneer/testk

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
0 Code Smells

No Coverage information
0.0% Duplication

[nipafx]

This is really cool and I love how far you've taken the simplistic "let's have AssertJ-esque assertions"-idea. I'm really looking forward to start using these assertions. 👍

That said, we agreed (on stream) that the ExecutionResultAssert::hasNumberOfTests/etc methods are a bit confusing. They appear to be asserting the number of tests, but they aren't. The problem described in TestAssert has the same underlying reason:

 * There is an inherent problem with the naming of the `thatStarted()` method that you should be aware of:
 * Imagine the following scenario: You have 4 tests, 3 should fail, one should succeed.
 * The logical way to test that would be to write
 * <p>
 *     assertThat(results).hasNumberOfTests(3).thatStarted().thenFailed();
 *     assertThat(results).hasSingleTest().thatStarted().thenSucceeded();
 * </p>
 * Looks good, does not work: it fails on the first line because there were actually 4 tests that started.

The only solution we found so far is to "flatten" the API and provide detailed assertion methods directly on ExecutionResultAssert or TestAssert. The latter would look as follows:

interface ExecutionResultAssert {
	
	TestAssert tests();

	// same for container (and report entries?)
	
}

interface TestAssert {
	
	TestAssert hasTests(int ex);
	
	TestAssert hasSuccessfulTests(int ex);
	
	FailedTestAssert hasFailedTests(int ex);
	
}

interface FailedTestAssert extends TestAssert  {

	AbstractThrowableAssert<?, ? extends Throwable> withException(Class<? extends Throwable> exceptionType);

	AbstractThrowableAssert<?, ? extends Throwable> withException();
	
}

This would bloat TestAssert by making all combinations of state/cardinality explicit methods. Yet, it seems to be the best solution.

[nipafx]

Also, interface implementations should probably all be package-visible and I'm not convinced about EntryPoint. 🧐

[Bukama]

Please merge the current master into the branch, to remove the Gradle issues mentioned in #290 - thank you!

[nipafx]

Looks good to me. Proposed commit message:

Create assertions for Pioneer (#232 / #245)

In 0b79db8 (#6 / #218), we moved all assertion-like methods onto the
`ExecutionResults` class even though that wasn't a good fit. The
intention was to collect all such methods to then more easily replace
them with proper AssertJ-like assertions that we needed to write
ourselves.

This change implements these methods. The API is pretty good already,
but we expect that after using it for a while the experience with it
as well as new use cases may lead to further changes (see #298).

Closes: #232
PR: #245

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
0 Code Smells

No Coverage information
0.0% Duplication