Fwd: [concordion] Example Command and @Before / @After

[Tim Wright]

Hi all,

Interesting and obscure bug/feature I've just identified with our jUnit integration.

* jUnit creates a new fixture object for each test case.

* Concordion does not create a new fixture object for each example (or "test case" because our examples appear as test cases to jUnit).

I am investigating why.

But: does anyone have a strong feeling either way? I lean toward: we do things the same as jUnit for consistency reasons.

Tim

---------- Forwarded message ----------
From: Tim Wright <t...@tfwright.co.nz>
Date: 23 November 2015 at 18:49
Subject: Re: [concordion] Example Command and @Before / @After
To: Andrew <andrew...@xtra.co.nz>
Cc: concordion <conco...@googlegroups.com>

Hey,

Just a note - concordion specifications using the example command do *not* create a new fixture object for each example. For some reason we are bypassing the junit fixture object creation code. Not sure if this is a bug or not  - I'll confer with the concordion-dev list.

Tim

On 23 November 2015 at 18:41, Tim Wright <t...@tfwright.co.nz> wrote:

Hi all,

Just to confirm: junit does use a new fixture object for each test (or example). The following test prints "1 1". I am currently checking that the same behaviour occurs in a concordion spec using examples.

import org.junit.Test;

import java.util.concurrent.atomic.AtomicInteger;

/**
 * Created by tim on 23/11/15.
 */
public class TestIndependance {

    private AtomicInteger i = new AtomicInteger(0);

    @Test
    public void testOne() {
        System.err.println(i.addAndGet(1));
    }

    @Test
    public void testTwo() {
        System.err.println(i.addAndGet(1));
    }

}

On 23 November 2015 at 18:29, Tim Wright <t...@tfwright.co.nz> wrote:

Hi Andrew,

TLDR version: it might be best *not* to use the example functionality in your test cases.

I've often wondered why jUnit's BeforeClass and AfterClass annotations are static - sometimes it causes issues. I believe that the rationale is that we shouldn't be sharing data between tests because it risks creating ordering effects - it's better to recreate all test data for each test case (or each "example" as we call them in concordion).

In fact, Martin Fowler (one of the jUnit authors) writes about this exact point:

http://martinfowler.com/bliki/JunitNewInstance.html

I've also done some research and discovered that jUnit creates a new test object for *each* test case. It used to be that a concordion test had exactly one test case - now we have one per example. So a non-static before spec annotation might not work for you. Note that I have not observed this in my own jUnit usage so the jUnit behaviour might have changed over time.

http://stackoverflow.com/questions/1052577/why-must-junits-fixturesetup-be-static

eg: apparently the following code prints "0 0 0":

public class TestJunit
{

    int count = 0;

    @Test
    public void testInc1(){
        System.out.println(count++);
    }

    @Test
    public void testInc2(){
        System.out.println(count++);
    }

    @Test
    public void testInc3(){
        System.out.println(count++);
    }
}

However, pragmatically :)

• I do like the idea of beforeProcessingSpecification and afterProcessingSpecification annotations. These are like Spring's @PostConstruct and @PreDestroy annotations. We could even have concordion:example="beforeProcessingSpecification" tags in the spec - like concordion:example="before" tags. However, I've looked at the code (including our junit integration) and it might be tricky to do cleanly.
  
  
• You are right that this.getClass().getName() will *not* work the same static methods as it does in non-static methods. You can get around this by having the static methods take a class parameter. But that won't work for @BeforeClass methods where you can't pass parameters. Sigh.
  
  
• One way to get around the @before annotation working differently is to create lazy initialisers. That way the initializers are only invoked once. Something like the following - but I can't figure out a good @AfterClass version.

public synchronized void before() {

  if (!initialized) {

    initialised=  true;

    ... do whatever

  }

• If you only have one example (ie: don't use the concordion:example tag) then @Before and @After will work as they did before :)

Hope that helps,

Tim

On 23 November 2015 at 14:19, Andrew <andrew...@xtra.co.nz> wrote:

Currently we do a number of setup/teardown actions for the entire specification using the Junit @Before and @After methods, crucially we use @Before in the specification to setup test data belonging to the specification, and @After in the base fixture to ensure this data is cleaned up, the browser is closed, etc.

Now that the @Before and @After methods are called for each example this isn't working.  I had a quick look at @BeforeClass/AfterClass but:

• I don't believe tricks like using "this.getClass().getName()" to get the name of the test class from the base fixture will work on a static method
• If I changed the test data store to a static then I'm concerned about this bleeding into other tests, especially if we start running tests in parallel

Any suggestions on how to work around this?

My current thought is to create two new annotations @BeforeProcesssingSpecification and @AfterProcesssingSpecification and ensure any method decorated with those get called at the appropriate time.  This would be better than @Before/After as currently if any errors are thrown in @Before then the specification is not generated (this might be a by product of using @FailFast though, I haven't looked at the issue to closely).  

Cheers

  Andrew

--
You received this message because you are subscribed to the Google Groups "concordion" group.
To unsubscribe from this group and stop receiving emails from it, send an email to concordion+...@googlegroups.com.
To post to this group, send email to conco...@googlegroups.com.
Visit this group at http://groups.google.com/group/concordion.
To view this discussion on the web, visit https://groups.google.com/d/msgid/concordion/a18c01ce-0533-4aed-a05e-b6223e724c86%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

--

Tim

021 251 5593

http://www.linkedin.com/in/drtimwright 

--

Tim

021 251 5593

http://www.linkedin.com/in/drtimwright 

--

Tim

021 251 5593

http://www.linkedin.com/in/drtimwright 

--

Tim

021 251 5593

http://www.linkedin.com/in/drtimwright 

[andrew...@xtra.co.nz]

My concern with moving to the "correct" way of doing things is that it may be a backwards incompatible change that make it harder to migrate existing test suites over to 2.0's Example Command (as I have just found out on my current project). 

I have only ever really thought of junit as a convenient vehicle to run the tests and that it is the specification that is the king.

Breaking examples down into separate junit tests brings so much, but I would still prefer to be able to treat the specification as a whole for setup/teardown in some/most instances - particularly given how the developers have structured the test cases on my current project: basically they're using @before to setup all the data for the various examples and @after to clean up afterwards.  Personally I would have preferred they setup the data during the first step of each example but the current approach does work quite well. 

Additionally: we're using the concordion-logback-extension and calling it from @before and @after methods to direct logging to specification specific log files (not sure what to do about examples yet as I've been concentrating on the storyboard extension).  This is another instance where I need some way of either hooking into before/afterSpecificationProcessing events and have the fixture remain the same across all examples, or completely redesign the approach.

I realise I'm not being very pure but it would make my things somewhat easier for me :-)

Does the parallel runner add any complications to this? I don't know if it runs examples in parallel or just specifications.

Andrew

[Nigel Charman]

Just to clarify the situation. Current specifications will be executed the same way with Concordion 2.

If you update the specification to use the new concordion:example command:

1. specification variables (the variables in the Concordion spec starting with #) are now scoped to the example
2. @Before and @After will be called for each example
3. Currently, the fixture instance state is not reset for each example

I lean towards resetting the fixture instance state for each example. Sharing state between examples is generally considered a bad practice, and test frameworks such as JUnit and Cucumber purposely restrict it. Since we are now enabling the running of single examples (eg. after failing of an example), users should be able to run that example in isolation without needing prior examples to have run.

If we implement this, we will need to make sure this is well documented.

Another factor to consider is that JUnit 5 will change things. Their wiki documents the current thinking, with the prototype including new annotations @BeforeEach, @AfterEach, @BeforeAll and @AfterAll with a @TestInstance(PER_CLASS) annotation to override the default behaviour and have the test instance retained across test methods.

Nigel.

PS. The alpha of JUnit 5 is due in Feb. It includes major changes to the way that Test Engines plug in to JUnit. I've created issue 141 to track it.
PPS. this won't fix Andrew's problem. If anything it will exacerbate it.

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 http://groups.google.com/group/concordion-dev.
To view this discussion on the web, visit https://groups.google.com/d/msgid/concordion-dev/CAJifTyrz8HZ4-8GL%2BMH2PQpkfC5Svqeh2fpxRDQr7qJwn2nRTw%40mail.gmail.com.

[Nigel Charman]

Comments inline..

On 23/11/15 22:14, andrew...@xtra.co.nz wrote:
> My concern with moving to the "correct" way of doing things is that
> it may be a backwards incompatible change

I'd disagree that it's backwards incompatible, since there will be no
change to how current specifications are run.

> that make it harder to migrate existing test suites over to 2.0's
> Example Command (as I have just found out on my current project).

Agreed, it will make it harder to add the new example command if your
existing suites are sharing state between examples.

> I have only ever really thought of junit as a convenient vehicle to
> run the tests and that it is the specification that is the king.

Agreed, I wouldn't want us to tie ourselves too much to JUnit. I know
people have implemented other runners, eg TestNG.

> Breaking examples down into separate junit tests brings so much, but I
> would still prefer to be able to treat the specification as a
> whole for setup/teardown in some/most instances - particularly given
> how the developers have structured the test cases on my current
> project: basically they're using @before to setup all the data for the
> various examples and @after to clean up afterwards. Personally I
> would have preferred they setup the data during the first step of each
> example but the current approach does work quite well.

To maintain what they currently have, couldn't they just change to using
@BeforeClass/@AfterClass and static state?

Another option would be to create a before example, which will be run
before each example.

> Additionally: we're using the concordion-logback-extension and calling
> it from @before and @after methods to direct logging to specification
> specific log files (not sure what to do about examples yet as I've
> been concentrating on the storyboard extension). This is another
> instance where I need some way of either hooking into
> before/afterSpecificationProcessing events and have the fixture remain
> the same across all examples, or completely redesign the approach.
>
> I realise I'm not being very pure but it would make my things somewhat
> easier for me :-)
>
> Does the parallel runner add any complications to this? I don't know
> if it runs examples in parallel or just specifications.

The current parallel runner just runs specifications in parallel. At the
moment, there are no plans to support running examples in parallel.
However, it could be that you can configure a JUnit runner to run test
methods in parallel, and that *might* work. I don't think it's been tried!
>
> Andrew
Nigel.

[andrew...@xtra.co.nz]

I can't help thinking that creating a new fixture object per example could lead to some strange behaviour - this would need to be really well documented.  When you're dealing with a junit test suite and each test is annotated with @test it makes sense that each test is standalone.  When you're dealing with a specification and fixture the lines become very blurred and I think you may end up making developing these tests more complicated than it needs to be.  That said I'd really need to try it out and see what impacts it had.

I also like to keep things as simple as possible so that it's easier to cross train testers so that they can help develop the tests so I have a vested interest in keeping things simple :-)

From your previous comments:

To maintain what they currently have, couldn't they just change to using @BeforeClass/@AfterClass and static state?

That would be fine if the code was in the fixture, but the majority of this code is in a base class all the test classes inherit from, eg we have a test data store that keeps track of any data added for the tests and cleans up after they've finished - which would still work except for the fact that we create this data once in an @before method in the fixture for all the examples to share rather than as each example is about to be executed.  A lot of that data is read only data that will not change with each example so the approach works well for us. 

Other code in the base fixtures @before/after methods uses this.getclass().getname() for things like setting up logging to a unique file per test and that just won't work with @Before/AfterClass static methods

 

 

From your other post: I also like the idea of @BeforeSpecification and @AfterSpecification annotations (and possibly also @BeforeSuite and @AfterSuite for the whole Concordion suite). This might be something you could try out as an extension and then report back on it?

I would be happy to but if current behaviour is changed so that a new fixture object is created for each example then I'm not sure how much benefit it would provide (for my current needs) as unless I'm using statics I'd loose any state information rendering the exercise pointless.

I'm not adverse to changing the tests to work with a new fixture object per example but I'm getting very concerned that the approach/framework that I've been building at MSD will be incompatible with the example command.  For example, how would you suggest we could use the logback extensions to get a separate log file per specification when have several examples?  (You'll probably need review some of my code to answer that one...)

• Perhaps we need the flexibility of something like the @TestInstance(PER_CLASS) annotation you mentioned?
• Or if there was always one fixture object created for the specification (plus a new one for each example) and we had the @Before/AfterSpecificaion support this might solve some of my issues. 

Andrew

[Nigel Charman]

Thanks Andrew, comments inline:

On 24/11/15 00:25, andrew...@xtra.co.nz wrote:

I can't help thinking that creating a new fixture object per example could lead to some strange behaviour - this would need to be really well documented.  When you're dealing with a junit test suite and each test is annotated with @test it makes sense that each test is standalone.  When you're dealing with a specification and fixture the lines become very blurred and I think you may end up making developing these tests more complicated than it needs to be.  That said I'd really need to try it out and see what impacts it had.

I also like to keep things as simple as possible so that it's easier to cross train testers so that they can help develop the tests so I have a vested interest in keeping things simple :-)

Agreed, but I don't want to compromise the solution just to make it simple. Let's look for a clean solution and then work to make it as easy as possible to use.

To maintain what they currently have, couldn't they just change to using @BeforeClass/@AfterClass and static state?

That would be fine if the code was in the fixture, but the majority of this code is in a base class all the test classes inherit from, eg we have a test data store that keeps track of any data added for the tests and cleans up after they've finished - which would still work except for the fact that we create this data once in an @before method in the fixture for all the examples to share rather than as each example is about to be executed.  A lot of that data is read only data that will not change with each example so the approach works well for us. 

What would be the downside of using a static data store, preferably using ThreadLocal to allow for future parallelisation of your tests?

Other code in the base fixtures @before/after methods uses this.getclass().getname() for things like setting up logging to a unique file per test and that just won't work with @Before/AfterClass static methods

I've found a thread here that discusses how to get the instance name from a static method.

A couple of options are:

Thread.currentThread().getStackTrace()[1].getClassNam
e();

or in Java 7+:

MethodHandles.lookup().lookupClass()

There is some performance penalty to both of these, so it might be worth trialling which is faster.

From your other post: I also like the idea of @BeforeSpecification and @AfterSpecification annotations (and possibly also @BeforeSuite and @AfterSuite for the whole Concordion suite). This might be something you could try out as an extension and then report back on it?

I would be happy to but if current behaviour is changed so that a new fixture object is created for each example then I'm not sure how much benefit it would provide (for my current needs) as unless I'm using statics I'd loose any state information rendering the exercise pointless.

I'm not adverse to changing the tests to work with a new fixture object per example but I'm getting very concerned that the approach/framework that I've been building at MSD will be incompatible with the example command.  For example, how would you suggest we could use the logback extensions to get a separate log file per specification when have several examples?  (You'll probably need review some of my code to answer that one...)

I'd be happy to :)

• Perhaps we need the flexibility of something like the @TestInstance(PER_CLASS) annotation you mentioned?

I'd rather we look for a cleaner solution than that.

• Or if there was always one fixture object created for the specification (plus a new one for each example) and we had the @Before/AfterSpecificaion support this might solve some of my issues.
  

That could work. We'd need some way to refer to the "specification object". Or we could use a @PerSpecification annotation to specify fields that you want retained across the whole specification.

Andrew

[andrew...@xtra.co.nz]

I had a go with these from @BeforeClass but neither worked :-(

Thread.currentThread().getStackTrace()[1].getClassName();

MethodHandles.lookup().lookupClass()

[Tim Wright]

I think they would only work when a static method in your fixture class is calling a static method elsewhere - then the static method elsewhere can determine the class of the object that called it.

If jUnit is calling a @BeforeClass method in a base class of your fixture, then I belive you'll just get a jUnit class returned by those.

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-de...@googlegroups.com.
To post to this group, send email to concord...@googlegroups.com.
Visit this group at http://groups.google.com/group/concordion-dev.
To view this discussion on the web, visit https://groups.google.com/d/msgid/concordion-dev/be40d947-697d-4d1f-8412-226cdb5ed082%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

[Nigel Charman]

Thanks for looking at this, and apologies it didn't work.

We've got a meeting next week to go through the questions on this thread and will report back.

Nigel.

On 26/11/15 10:07, Tim Wright wrote: