Concordion PR

77 views
Skip to first unread message

Tim Wright

unread,
May 10, 2018, 1:17:13 PM5/10/18
to concordion-dev

Hey everyone,

Just noticed this PR on our great software. It basically lets the user write code that selectively disable tests based on, well, anything. The use case I could think of is build step specific - in case some tests are unreliable or slow.
I've reviewed it and think it's a good idea. But it could be even better - by letting the user have code that arbitrarily changes the implementation status.

Tim

Nigel Charman

unread,
May 10, 2018, 3:40:28 PM5/10/18
to concord...@googlegroups.com
It's an interesting concept. Having an extension method provides a lot of flexibility. It does make it a compile-time rather than run-time option though. Any thoughts on how it might work if a user wanted to change at run-time? Are there scripting options that would allow this?

I quite like having a SKIPPED status. Rather than being unimplemented, expected to fail or expected to pass, you might just want to skip a test (eg. it takes too long, or isn't pertinent).

I agree with Tim, it would be great if it could also set Expected to Fail, Expected to Pass or Unimplemented. One request we had in the past was to add an expiry date onto Expected to Fail. This filter would allow that option if extended to support all implementation statuses.

There's a couple of other items in progress that may need to be merged/considered along with this change:
  1. PR#264 has some breaking API changes. I've been doing some exploratory work to clean this up further. If changing the API, it would make sense to change these together.
  2. Luke Pearson is working on an expected to fail info extension that adds text to describe why an example is expected to fail. If adding skipped examples, we should consider changing this extension to allow a description to be added to any implementation status. Would also be worth considering whether this becomes part of core, or stays as an extension? I quite like it as an extension since it opens up options for how you display/log the text.

Nigel

--
You received this message because you are subscribed to the Google Groups "concordion-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to concordion-de...@googlegroups.com.
To post to this group, send email to concord...@googlegroups.com.
Visit this group at https://groups.google.com/group/concordion-dev.
To view this discussion on the web, visit https://groups.google.com/d/msgid/concordion-dev/CAJifTyoNrf-Nhp6VpvC50QZmKv5AcNQmmQffYsQvcS%3DvD69%2BHQ%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.

Tim Wright

unread,
May 10, 2018, 3:46:13 PM5/10/18
to Nigel Charman, concordion-dev

Hey,

By 'run time' what I meant is "the decision on implementation status can be made using information available at run time - like environment variables, java version, operating system, date, etc".

Tim


On 11 May 2018 at 07:40, Nigel Charman <nigel.ch...@gmail.com> wrote:
It's an interesting concept. Having an extension method provides a lot of flexibility. It does make it a compile-time rather than run-time option though. Any thoughts on how it might work if a user wanted to change at run-time? Are there scripting options that would allow this?

I quite like having a SKIPPED status. Rather than being unimplemented, expected to fail or expected to pass, you might just want to skip a test (eg. it takes too long, or isn't pertinent).

I agree with Tim, it would be great if it could also set Expected to Fail, Expected to Pass or Unimplemented. One request we had in the past was to add an expiry date onto Expected to Fail. This filter would allow that option if extended to support all implementation statuses.

There's a couple of other items in progress that may need to be merged/considered along with this change:
  1. PR#264 has some breaking API changes. I've been doing some exploratory work to clean this up further. If changing the API, it would make sense to change these together.
  2. Luke Pearson is working on an expected to fail info extension that adds text to describe why an example is expected to fail. If adding skipped examples, we should consider changing this extension to allow a description to be added to any implementation status. Would also be worth considering whether this becomes part of core, or stays as an extension? I quite like it as an extension since it opens up options for how you display/log the text.

Nigel


On 11/05/18 5:17 AM, Tim Wright wrote:

Hey everyone,

Just noticed this PR on our great software. It basically lets the user write code that selectively disable tests based on, well, anything. The use case I could think of is build step specific - in case some tests are unreliable or slow.
I've reviewed it and think it's a good idea. But it could be even better - by letting the user have code that arbitrarily changes the implementation status.

Tim

--
You received this message because you are subscribed to the Google Groups "concordion-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to concordion-dev+unsubscribe@googlegroups.com.
To post to this group, send email to concordion-dev@googlegroups.com.

--
You received this message because you are subscribed to the Google Groups "concordion-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to concordion-dev+unsubscribe@googlegroups.com.
To post to this group, send email to concordion-dev@googlegroups.com.

Visit this group at https://groups.google.com/group/concordion-dev.
To view this discussion on the web, visit https://groups.google.com/d/msgid/concordion-dev/e74831ff-5502-cac1-b5f0-fbd507a8e6f3%40gmail.com.

Tim Wright

unread,
May 10, 2018, 3:47:20 PM5/10/18
to Nigel Charman, concordion-dev
And I do like having a description to be added to the implementation status of any test - or just to any test :)


To post to this group, send email to concord...@googlegroups.com.

--
You received this message because you are subscribed to the Google Groups "concordion-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to concordion-dev+unsubscribe@googlegroups.com.
To post to this group, send email to concord...@googlegroups.com.

ian bondoc

unread,
May 11, 2018, 3:24:41 AM5/11/18
to concordion-dev
Hi Tim,

I've posted a reply to the PR earlier because I cannot reply here at work time.

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 whole message in the PR suggests the idea of removing the SKIPPED/IGNORED from Implementation status.

I was thinking of moving the logic of skipping/ignoring an example in FixtureRunner.run and change the parameters from example name and fixture to ConcordionFrameworkMethod and fixture.  And within the run method, the filter can skip executing the example and call notifier.fireTestIgnored().

What do you think?

Nigel Charman

unread,
May 12, 2018, 9:52:53 PM5/12/18
to concordion-dev
This has opened up discussion on whether there should also be a similar facility for Unimplemented/ExpectedToFail/ExpectedToPass. From the PR:

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.
... 
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? 

There may be a use case for ExpectedToFail - factors could include OS, date/time or other environmental factors. For example, an example or specification may be expected to fail on Windows, but pass on OS/X. The difference between skipping and expecting it to fail is that it would still run in the latter case, and cause a JUnit failure if the example passes. 

I think Unimplemented is more obtuse since a specification either has assertions or not. The only way I know of modifying this currently is the ExecuteOnlyIf extension, which applies conditional logic to parts of a specification.

Keen for other thoughts on selectively modifying the status of ExpectedToFail/ExpectedToPass/Unimplemented?

Nigel Charman

unread,
May 12, 2018, 9:55:03 PM5/12/18
to concordion-dev
Hi Ian

Once this PR is merged, it would be great if you would extend our Spring integration documentation to include details, along with any other Spring integration tips you have. We already reference your Spring runner :)

On Friday, 11 May 2018 19:24:41 UTC+12, ian bondoc wrote:
...

ian bondoc

unread,
May 12, 2018, 10:41:03 PM5/12/18
to concordion-dev
Hi Nigel,

Good point on the use case of marking as ExpectedToFail due to os, date/time, etc.  Will be applying the changes as advised by Tim:
- use @Ignore
- use a generic filter which would be named ImplementationStatusModifier which would return an ImplementationStatus based on the example element
- change the logic of example command to depend on implementation status
- derived ImplementationStatus would override what is declared in xml via c:status param
- let the example behave like @Ignore annotated child spec if ignored
- extend test coverage to count all including ignored

Nigel/Tim are you ok with these changes?

About spring integration, will do (I actually updated the runner to support new spring @IfProfileValue)

Tim Wright

unread,
May 12, 2018, 10:46:56 PM5/12/18
to ian bondoc, concordion-dev
That shoulda good to me! And hey Ian - I just realised now that we used to work together at verifone :) are you still there?

I still see it as an api breaking change - it is a new method on an interface. So if any of our extension developers have implemented that interface, their code won’t compile anymore.

Tim

Tim
021 251 5593
Sent from phone.
From: concord...@googlegroups.com <concord...@googlegroups.com> on behalf of ian bondoc <chik...@gmail.com>
Sent: Sunday, May 13, 2018 2:41:03 PM
To: concordion-dev
Subject: [concordion-dev] Re: Concordion PR
 
--
You received this message because you are subscribed to the Google Groups "concordion-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to concordion-de...@googlegroups.com.

To post to this group, send email to concord...@googlegroups.com.
Visit this group at https://groups.google.com/group/concordion-dev.

ian bondoc

unread,
May 12, 2018, 11:25:27 PM5/12/18
to concordion-dev
Hi Tim,

Agree with your point on the version.  Anyway version management is up to you :).

Re verifone, no I moved already.

Will go ahead with the changes then.  Thanks for the review!
To post to this group, send email to concor...@googlegroups.com.

Tim Wright

unread,
May 12, 2018, 11:32:06 PM5/12/18
to ian bondoc, concordion-dev
If only the java version we use had support for default methods on interfaces :)

Tim
021 251 5593
Sent from phone.
Sent: Sunday, May 13, 2018 3:25:26 PM
To: concordion-dev
Subject: Re: [concordion-dev] Re: Concordion PR
 
To post to this group, send email to concord...@googlegroups.com.

Nigel Charman

unread,
May 13, 2018, 4:47:29 AM5/13/18
to Tim Wright, ian bondoc, concordion-dev
It must be about time to only support Java 8+. JUnit 5 and Selenium WebDriver have both made the move. How about we finish the JUnit 5 support and move to Java 8 :)

PS. Based on current usage, I suggest we define the versioning system for Concordion as MAJOR.MINOR.PATCH where:
* MAJOR = significant change for everyday users of Concordion (eg. addition of Markdown, possibly JUnit 5 or JDK upgrade)
* MINOR = incompatible API change for extension developers or new functionality
* PATCH = backwards compatible bug fix

ian bondoc

unread,
May 18, 2018, 7:02:08 AM5/18/18
to concordion-dev
Hi,

In case you were not notified by the commit in the PR I'm just mentioning it here that it's ready for review/merge :)


On Friday, May 11, 2018 at 5:17:13 AM UTC+12, Tim Wright wrote:

Nigel Charman

unread,
Jun 3, 2018, 7:41:39 AM6/3/18
to concordion-dev
Hi

We've got a few changes lurking that I'm tempted to bundle together into a Concordion 3.0 release:

* this PR#267
* replace Pegdown with Flexmark markdown parser - I'm about half way through this
* fix an issue with @BeforeExample and @AfterExample (PR#264)
* JUnit 5 integration (PR#234)
* remove JUnit3 integration
* require JDK8

One thing I'm wondering is whether we'll also be able to utilise the extension point in this PR to implement JUnit5's conditional test execution (see 4.2 of http://www.baeldung.com/junit-5-extensions)? We'll need to do some more work on PR#234 too.

Nigel

ian bondoc

unread,
Jun 6, 2018, 2:42:43 AM6/6/18
to concordion-dev
Hi Nigel,

The initial intent of the PR is to provide this feature to the current version of Concordion (2.x).  If Concordion 3.0 would be junit5 compatible then this PR doesn't add any value.  The junit5 extension hook for conditional tests already addresses the same concern as this PR (even at example level)

Would it be acceptable for us to just merge it with 2.x stream for now and adjust if needed for junit5 compatibility?

Nigel Charman

unread,
Jun 10, 2018, 3:17:53 AM6/10/18
to concordion-dev
Hi Ian

That's not a bad idea. Possibly a 2.2.0 release containing this PR and PR#264, and deferring the other changes to 3.0. The release will have some breaking API changes that could impact extensions.

JUnit 5 changes will need a fair bit of experimentation before releasing, so 3.0 could take a fair bit of time to shape up. I'll review the PR further this week.

cheers
Nigel
Reply all
Reply to author
Forward
0 new messages