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

Add ExpectedToFail test extension (#551 / #676) #676

Merged

Conversation

Marcono1234
Copy link
Contributor

@Marcono1234 Marcono1234 commented Oct 2, 2022

Proposed commit message:

Add ExpectedToFail test extension (#551 / #676)

PR: #676

Supersedes #546 (based on the same commits of that PR):

  • I did not override 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.
  • As already mentioned in the comments of Add NotWorking test extension (#551 / #546) #546, handling for exceptions which occur in @AfterEach could not be kept with the InvocationInterceptor approach.
  • Due to using 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)

  • There is documentation (Javadoc and site documentation; added or updated)
  • There is implementation information to describe why a non-obvious source code / solution got implemented
  • Site documentation has its own .adoc file in the docs folder, e.g. docs/report-entries.adoc
  • Site documentation in .adoc file references demo in src/demo/java instead of containing code blocks as text
  • Only one sentence per line (especially in .adoc files)
  • Javadoc uses formal style, while sites documentation may use informal style

Documentation (new extension)

  • The docs/docs-nav.yml navigation has an entry for the new extension
  • The package-info.java contains information about the new extension

Code

  • Code adheres to code style, naming conventions etc.
  • Successful tests cover all changes
  • There are checks which validate correct / false usage / configuration of a functionality and there are tests to verify those checks
  • Tests use AssertJ or our own PioneerAssert (which are based on AssertJ)

Contributing

  • A prepared commit message exists
  • The list of contributions inside README.md mentions the new contribution (real name optional)

Note that this loses the ability to properly handle exceptions from
AfterEach methods.
@Marcono1234 Marcono1234 force-pushed the feature/expected-to-fail-extension branch from ddf72ad to 5ecce32 Compare October 2, 2022 15:04
return TestAbortedException.class.isInstance(t);
}

private static <T> T invokeAndInvertResult(Invocation<T> invocation, ExtensionContext extensionContext)
Copy link
Contributor Author

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.

@Michael1993 Michael1993 added the full-build Triggers full build suite on PR label Oct 2, 2022
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
Copy link
Member

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?

Copy link
Contributor Author

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:

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.

@nipafx nipafx linked an issue Nov 9, 2022 that may be closed by this pull request
@nipafx
Copy link
Member

nipafx commented Nov 9, 2022

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. 👍🏾

@nipafx
Copy link
Member

nipafx commented Nov 9, 2022

Proposed commit message:

Add `@ExpectedToFail` test extension (#551 / #676)

As opposed to JUnit Jupiter's `@Disabled` annotation, this extension
executes the annotated test and then effectively flips the result:

* failed ~> ignored
* passed ~> failed

This ensures that a temporarily disabled test will get discovered
once the reason for being disabled no longer holds, so it can/must
be re-enabled by removing `@ExpectedToFail`.

PR: #676

@nipafx
Copy link
Member

nipafx commented Nov 9, 2022

@Marcono1234 Forgot to write: Can you let us know whether the changes I made are ok? I'll wait with the merge until then.

Copy link
Contributor Author

@Marcono1234 Marcono1234 left a 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.

docs/expected-to-fail-tests.adoc Outdated Show resolved Hide resolved
Co-authored-by: Marcono1234 <Marcono1234@users.noreply.github.com>
@nipafx
Copy link
Member

nipafx commented Nov 11, 2022

Thank you, @Marcono1234! 👍🏾

@nipafx nipafx merged commit 8b284b8 into junit-pioneer:main Nov 11, 2022
@nipafx
Copy link
Member

nipafx commented Nov 11, 2022

Thanks again, for your high-quality work and patience. I just released 1.8.0 with your extensions. 🙌🏾

@Marcono1234 Marcono1234 deleted the feature/expected-to-fail-extension branch November 13, 2022 22:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
full-build Triggers full build suite on PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide annotation that a test currently fails by given circumstances
3 participants