All committers have signed the CLA.

This seems like a sensible PR - there's clearly a use case for it. I'd prefer for the feature to be named IGNORED instead of SKIPPED because that matches the jUnit @ignore annotation better. Now to read the code!

The initial intent was to use JUnit's @ignore annotation, however it would require being consistent with JUnit's behaviour when ignoring a linked specification (which has some inconsistencies with current Unimplemented and ExpectedToFail).

• should the Ignored count be set to 1 for each example?
• should the note <-- Note: This example has been marked as IGNORED be removed from logs and html?
• should the JUnit's RunNotifier.fireTestIgnored be invoked for each ignored example?
  Tnx!

Have now finished reviewing the PR. I like the idea - There is an interesting use case for disabling a different set of tests at runtime - so that a user can run different concordion tests locally, in PR builds, or in deploys - in case some tests are slow or unreliable in a particular build pipeline step. We do that at work right now (but sadly don't use concordion at my current job!)

Here are my more specific thoughts:

• We'll need to contemplate how to release the code as it introduces a new method in an API interface.
• I think for language consistency we should change "Skipped" to "Ignored" in the code
• We should consider making the code slightly more generic so the ExampleFilter can override the implementation status with any implementation status (expected to fail, expected to pass, ignored, etc)
• That would make it not so much an ExampleFilter as a "ExampleImplementationStatusModifier"
• That change would also move the logic for the skipping to the setupCommandForExample method in the same file.
• Because we're introducing a breaking API change it's worth contemplating the more flexible use case here. But I hate overdesign :)

I've started a convo on the concordion-dev mailing list to chat about those thoughts. It'd be great if you joined so that we can chat about your PR together. Naturally we're super happy to have your PR and don't want to force you to do lots of work to get it merged!

Hopefully this link works: https://groups.google.com/forum/#!forum/concordion-dev

The main use case and motivation was to allow concordion to hook into spring's profile mechanism where examples can be tagged with a param (e.g. spring-profile='smoke-test, all') and conditionally run them based on the active profile or similar purpose of @IfProfileValue.

The semantic difference of Unimplemented/ExpectedToFail vs Ignored/Skipped is that the former refers to the state of the example while the latter refers to the result of the run. An example could be declared as Unimplemented/ExpectedToFail and still be ignored at runtime. The main difference is concordion allows Unimplemented/ExpectedToFail to execute while the latter would not. Considering this, IGNORED doesn't belong to the ImplementationStatus - the only reason it's there is for the additional notes in the logs and html.

I agree to use JUnit's @ignore, so it would be consistent with the behavior of an @ignore annotated fixture (though there's actually no way of annotating examples).

However, I disagree that Unimplemented/ExpectedToFail should be override-able/set-able at runtime as these are the state of tests - is there a use case for them to change in between runs given the same version of test and system being tested? Also, setupCommandForExample is called after the example has already ran (seems the code inside is meant to update the example element based on the ImplementationStatus).

Lastly, this is not a breaking change as it doesn't remove or modify any existing API so a minor version bump (signifying a new feature) would be appropriate.

Thanks for the invite, will be happy to discuss further in the forum later.

Committed all requested changes - for your review. Thanks!

As per the post on the concordion-dev list, I'm keen to also try this out with JUnit 5 before releasing it. We do need to do some more work on the JUnit 5 PR #243 too.
In the meantime, anyone that wants to pick this up can use Jitpack to add it to their dependencies.

I hope no one minds me poking my head in here, but I saw the discussion about using JUnit's @ignore annotation. I would be hesitant to tie functionality to it because JUnit 5 replaced it.

@Ignore no longer exists: use @Disabled instead.

IMO, it seems like it would be better for concordion to define its own API in this regard.

Edited for clarity

I agree @miralexan (and thanks for poking your head in).

We should remove this reliance on JUnit. Ideally, only the org.concordion.integration.junit* packages would rely on JUnit. We've got a bit of leakage into the DefaultConcordionRunner class that we could look to tidy up in Concordion 3.0, but that's all at the moment.

I'd suggest we make this a Concordion specific flag. If it will only apply to examples, and not specifications, can we use @IgnoreExample? This is similar to the @BeforeExample annotation that we introduced to remove reliance on JUnit's @Before (which we also saw was changing in JUnit5).

@miralexan I agree that's why the initial changes doesn't depend on JUnit lib. @nigelcharman will introduce the @IgnoreExample annotation just note that there's really no use for it as fixture methods doesn't really represent an example.

To add, this PR is targeted for Concordion 2.x only because 3.0 (being targeted for JUnit 5) won't need it - thus should be deprecated in that release.

@chiknrice I don't think that JUnit 5 provides this functionality. The @Disabled annotation can be applied at container or test level in JUnit 5, and causes the whole class or test to be ignored if it matches the condition.

This extension allows individual examples to be ignored. Why do you think JUnit 5 would deprecate it?

I've experimented with a few other cases:

• Changing status to EXPECTED_TO_FAIL works nicely :)
• Changing status of the [Outer] example does not work - I'd be keen for thoughts on whether this might be needed or we leave as a constraint
• Changing status of the before element results in an exception - this can be a constraint, I don't think we'd ever want this to happen
• The @BeforeExample, @AfterExample and Example Listeners are still executed for the ignored example - I think this is something we should look at

@nigelcharman what I mean by deprecating this feature in concordion 3.0 is because skipping the examples can be achieved already by JUnit 5 not via @Disabled but via ExecutionCondition api. The examples are translated as test methods under the covers anyway.

Possibly, at the moment, I'm thinking that the ImplementationStatusModifier gives us more flexibility now, so might just stick with what you have created under JUnit 5. Will revisit when we finish the JUnit 5 implementation.

@chiknrice a bit of tangent, but based on the current state of Junit 5 I think that it's unlikely that concordion would be able to leverage the ExecutionCondition API.

If concordion 3.0 is implemented as it's own Junit platform TestEngine then none of the Junit Jupiter APIs/extension points will be available. If concordion 3.0 is implemented as a Junit Jupiter extension, then examples will almost certainly be implemented as dynamic tests, but individual dynamic tests don't participate in the Junit Jupiter test lifecycle. That is, extension callbacks are invoked for the group of dynamic tests as a whole, but not individually. From the linked doc:

The execution lifecycle of a dynamic test is quite different than it is for a standard @Test case. Specifically, there are no lifecycle callbacks for individual dynamic tests. This means that @BeforeEach and @AfterEach methods and their corresponding extension callbacks are executed for the @TestFactory method but not for each dynamic test. In other words, if you access fields from the test instance within a lambda expression for a dynamic test, those fields will not be reset by callback methods or extensions between the execution of individual dynamic tests generated by the same @TestFactory method.

@miralexan thanks for pointing that out, wasn't aware of the concordion team's plans for 3.0 hence my continued assumption of how examples work. If that's the case then good - there's still a use for this PR in 3.0 :)

yes, fork is up to date, thanks!

OK, I've merged it. I'm finishing off #264 which has a number of breaking API changes and hope to release both at the same time - possibly a week or two away.
@chiknrice - what would you like in the release notes for this PR? Is there an example of using it with Spring you can point us too?
@drtimwright - let us know if you have any additional comments and we can change on master.

I guess just "New extension point to allow programmatically setting the implementation status of examples". I don't have an example at the moment, but I plan to create an extension leveraging spring profiles - will just link to it later. Thanks!