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
Add ExpectedToFail test extension (#551 / #676) #676
Add ExpectedToFail test extension (#551 / #676) #676
Conversation
591c524
to
ddf72ad
Compare
Note that this loses the ability to properly handle exceptions from AfterEach methods.
ddf72ad
to
5ecce32
Compare
return TestAbortedException.class.isInstance(t); | ||
} | ||
|
||
private static <T> T invokeAndInvertResult(Invocation<T> invocation, ExtensionContext extensionContext) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have implemented this as separate method in case more InvocationInterceptor
methods will be overridden in the future.
return assertSingleTest(actual.testEvents().dynamicallyRegistered().count()); | ||
Events testEvents = actual.testEvents(); | ||
assertSingleTest(testEvents.dynamicallyRegistered().count()); | ||
// Retain all events for returned Assert because TestCaseStartedAssert allows further filtering |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The double filtering was intentional. Could you provide your rationale for removing it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment was actually a bit misleading; I have now adjusted it.
The issue was that previously (in the current version of this project) the TestCaseAssertBase
instances were always created with all test / container events, without being restricted to the type of the hasSingle...()
method:
junit-pioneer/src/test/java/org/junitpioneer/testkit/assertion/PioneerAssert.java
Lines 119 to 122 in f36ee4c
private TestCaseAssertBase assertSingleTest(long numberOfTestsWithOutcome) { | |
try { | |
Assertions.assertThat(numberOfTestsWithOutcome).isEqualTo(1); | |
return new TestCaseAssertBase(actual.testEvents()); |
That happened to work because TestCaseAssertBase.throwable()
previously always restricted the events to failed()
, however this caused issues for this pull request when I implemented TestCaseAbortedAssert
.
Therefore most hasSingle...()
methods now create a TestCaseAssertBase
with properly filtered events, but for the methods where this is not possible (and which therefore behave the same way they did before) I added those comments.
Thanks for your work on this, @Marcono1234. I made some small changes (see 9731701, 11e03c2) and I think it's ready to be merged and released in version 1.8.0. 👍🏾 |
Proposed commit message:
|
@Marcono1234 Forgot to write: Can you let us know whether the changes I made are ok? I'll wait with the merge until then. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nipafx, thanks, the changes look fine to me! There only seems to be one small typo.
Feel free to merge this afterwards.
Co-authored-by: Marcono1234 <Marcono1234@users.noreply.github.com>
Thank you, @Marcono1234! 👍🏾 |
Thanks again, for your high-quality work and patience. I just released 1.8.0 with your extensions. 🙌🏾 |
Proposed commit message:
Supersedes #546 (based on the same commits of that PR):
InvocationInterceptor.interceptTestFactoryMethod
as originally proposed in Add NotWorking test extension (#551 / #546) #546 (comment) because that appears to be only the call to the factory method but not the individual test executions (?) so covering that probably does not make much sense.@AfterEach
could not be kept with theInvocationInterceptor
approach.InvocationInterceptor
this probably only supports@Test
methods now, but not any dynamic tests (have not investigated that in detail though).PR checklist
The following checklist shall help the PR's author, the reviewers and maintainers to ensure the quality of this project.
It is based on our contributors guidelines, especially the "writing code" section.
It shall help to check for completion of the listed points.
If a point does not apply to the given PR's changes, the corresponding entry can be simply marked as done.
Documentation (general)
.adoc
file in thedocs
folder, e.g.docs/report-entries.adoc
.adoc
file references demo insrc/demo/java
instead of containing code blocks as text.adoc
files)Documentation (new extension)
docs/docs-nav.yml
navigation has an entry for the new extensionpackage-info.java
contains information about the new extensionCode
Contributing
README.md
mentions the new contribution (real name optional)