state of an object in setUp() of junit TestCase

Discussion in 'Java' started by jimgardener, Oct 30, 2010.

  1. jimgardener

    jimgardener Guest

    If I forget to define a tearDown() method ,will an object initialized
    in setUp() maintain its state in between two test methods?
    suppose I am testing a Stopwatch class
    <code>
    public class Stopwatch {
    public long startTime=0;
    public long stopTime=0;
    boolean isrunning=false;
    public void start(){
    startTime=System.currentTimeMillis();
    isrunning=true;
    }
    public void stop(){
    if (isrunning) stopTime=System.currentTimeMillis();
    isrunning=false;
    }

    public long getTotalDurationMilliSeconds(){
    long duration=0;
    if( isrunning){
    duration=(System.currentTimeMillis()-startTime);
    }
    else{
    duration=stopTime-startTime;
    }
    return duration;
    }
    public void reset(){
    startTime=0;
    stopTime=0;
    isrunning=false;
    }

    public long getTotalDurationSeconds(){
    long duration=0;
    if( isrunning){
    duration=((System.currentTimeMillis()-startTime)/1000);
    }
    else{
    duration=(stopTime-startTime)/1000;
    }
    return duration;
    }
    }

    </code>

    I have a testcase like below,in which I have a setUp() but no
    tearDown()


    <code>
    class StopwatchTest extends TestCase{
    private Stopwatch watch;
    public StopwatchTest(String name){
    super(name);
    }
    public void setUp(){
    watch=new Stopwatch();
    }
    private void sleepForSomeTime(long timeinmillisecs){
    try{
    System.out.println("sleeping for :"+(timeinmillisecs/1000)+"secs");
    Thread.sleep(timeinmillisecs);
    }catch(InterruptedException e){

    }
    }
    public void testNormalOpButForgottToStop(){
    watch.start();
    sleepForSomeTime(3000);
    //watch is not stopped;
    assertEquals(3,watch.getTotalDurationSeconds());
    }
    public void testStoppedBeforeStart(){
    System.out.println("timeduration:"+watch.getTotalDurationSeconds());
    sleepForSomeTime(2000);
    watch.stop();
    System.out.println("timeduration:"+watch.getTotalDurationSeconds());
    assertEquals(0,watch.getTotalDurationSeconds());
    }

    }

    </code>

    It seems that the watch is initialized each time before running the
    test methods.Is this the expected behaviour?


    regards,
    jim
     
    jimgardener, Oct 30, 2010
    #1
    1. Advertising

  2. jimgardener

    jimgardener Guest


    > It seems that the watch is initialized each time before running the
    > test methods.Is this the expected behaviour?



    sorry..wrong wording..looks like the state of the object is reset each
    time before the test method
    jim
     
    jimgardener, Oct 30, 2010
    #2
    1. Advertising

  3. jimgardener

    Lew Guest

    You answered your own question, so I will just address some side issues.

    jimgardener wrote:
    > If I forget to define a tearDown() method ,will an object initialized
    > in setUp() maintain its state in between two test methods?
    > suppose I am testing a Stopwatch class
    > <code>
    > public class Stopwatch {
    > public long startTime=0;


    These are the default values. Explicitly setting them in member
    initialization causes them to be set twice. In the case of 'startTime' and
    'isrunning' [sic] you don't even use the initial value, so why bother setting it?

    > public long stopTime=0;
    > boolean isrunning=false;


    The variable name 'isrunning' violates the naming conventions.

    > public void start(){
    > startTime=System.currentTimeMillis();
    > isrunning=true;
    > }
    > public void stop(){
    > if (isrunning) stopTime=System.currentTimeMillis();


    This 'if' block violates the coding conventions.

    > isrunning=false;
    > }
    >
    > public long getTotalDurationMilliSeconds(){
    > long duration=0;


    You never use the '0' value, so why do you set it? Just so you can discard it?

    > if( isrunning){
    > duration=(System.currentTimeMillis()-startTime);
    > }
    > else{
    > duration=stopTime-startTime;
    > }
    > return duration;
    > }
    > public void reset(){
    > startTime=0;
    > stopTime=0;
    > isrunning=false;
    > }
    >
    > public long getTotalDurationSeconds(){
    > long duration=0;


    Again, why did you initialize 'duration' to a value that is immediately discarded?

    > if( isrunning){
    > duration=((System.currentTimeMillis()-startTime)/1000);
    > }
    > else{
    > duration=(stopTime-startTime)/1000;
    > }
    > return duration;
    > }
    > }
    >
    > </code>
    >
    > I have a testcase like below,in which I have a setUp() but no
    > tearDown()
    >
    >
    > <code>
    > class StopwatchTest extends TestCase{


    How come you aren't using '@Test'?

    > private Stopwatch watch;
    > public StopwatchTest(String name){
    > super(name);
    > }
    > public void setUp(){
    > watch=new Stopwatch();
    > }
    > private void sleepForSomeTime(long timeinmillisecs){


    The variable name 'timeinmillisecs' egregiously violates the naming standards.

    > try{
    > System.out.println("sleeping for :"+(timeinmillisecs/1000)+"secs");


    'System.out.println()'? Really? Come on.

    If it's important enough to log, it's important enough to log correctly.

    If you really are such a schlemiel that you have to use a
    'System.X.println()', use 'System.err' at least. But really, use a logger.

    > Thread.sleep(timeinmillisecs);
    > }catch(InterruptedException e){
    >
    > }
    > }
    > public void testNormalOpButForgottToStop(){
    > watch.start();
    > sleepForSomeTime(3000);
    > //watch is not stopped;
    > assertEquals(3,watch.getTotalDurationSeconds());
    > }


    This is not a valid test. Since the watch is not stopped and sleep times are
    approximate, you have a small risk that this assertion will fail even with
    correct code.

    The granularity of one second will make this rare, but not theoretically
    impossible. You should comment your code that you depend on the granularity
    to avoid trouble.

    > public void testStoppedBeforeStart(){
    > System.out.println("timeduration:"+watch.getTotalDurationSeconds());
    > sleepForSomeTime(2000);
    > watch.stop();
    > System.out.println("timeduration:"+watch.getTotalDurationSeconds());
    > assertEquals(0,watch.getTotalDurationSeconds());
    > }
    >
    > }
    >
    > </code>
    >
    > It seems that the watch is initialized each time before running the
    > test methods.Is this the expected behaviour?


    --
    Lew
     
    Lew, Oct 30, 2010
    #3
  4. jimgardener

    jimgardener Guest

    sorry..that was sloppy work on my side..Will be more careful..
    thanks Lew for the suggestions.
    regards
    jim
     
    jimgardener, Oct 30, 2010
    #4
  5. jimgardener

    Tom Anderson Guest

    On Sat, 30 Oct 2010, jimgardener wrote:

    > If I forget to define a tearDown() method ,will an object initialized
    > in setUp() maintain its state in between two test methods?


    No. JUnit creates a new instance of the test class for each test; if you
    write your test classes as you do (which is the right way to do it), then
    state won't leak from one test to the next, because there's a fresh set of
    instance variables each time.

    Note the if - if, on the other hand, you keep state in places which
    persist between instances (static variables, objects outside the test
    object), then it will leak from test to test. So don't do that.

    tom

    --
    Ed editor textorum probatissimus est -- Cicero, De officiis IV.7
     
    Tom Anderson, Oct 30, 2010
    #5
  6. Sat, 30 Oct 2010 10:55:22 -0400, /Lew/:
    > jimgardener wrote:
    >
    >> public class Stopwatch {
    >> public long startTime=0;

    >
    > These are the default values. Explicitly setting them in member
    > initialization causes them to be set twice.


    I don't like specifying initializers using the default values for
    the same reason, but I was once pointed to the Java Tutorial
    <http://download.oracle.com/javase/tutorial/java/nutsandbolts/datatypes.html>
    (see the "Default Values" section):

    > ... Relying on such default values, however, is generally
    > considered bad programming style.


    I don't completely agree with the given statement, but then I can't
    come up with arguments on any side. What do you think about it?

    --
    Stanimir
     
    Stanimir Stamenkov, Oct 30, 2010
    #6
  7. jimgardener

    markspace Guest

    On 10/30/2010 9:39 AM, Stanimir Stamenkov wrote:

    >> ... Relying on such default values, however, is generally
    >> considered bad programming style.

    >
    > I don't completely agree with the given statement, but then I can't come
    > up with arguments on any side. What do you think about it?



    Lew's comment that an explicit initialization requires a second,
    redundant and useless initialization is enough to counter Oracle's
    statement.

    I'd say precisely the opposite. The default values are 100% guaranteed
    and deterministic, there's no reason not to rely on them, and it's
    certainly not bad style. I'm sorry for whoever wrote that line in the
    tutorial but I think they went off some place unknown to most professionals.
     
    markspace, Oct 30, 2010
    #7
  8. jimgardener

    markspace Guest

    On 10/30/2010 9:24 AM, Tom Anderson wrote:

    > Note the if - if, on the other hand, you keep state in places which
    > persist between instances (static variables, objects outside the test
    > object), then it will leak from test to test. So don't do that.



    I've used instance fields but initialized them each time before a test.
    I don't know if this is good practice or not but there it is.

    Partial code:


    public class BasicIrcParserTest
    {

    private Writer writer;
    private MockCommandList commandList;
    private ChatScreen screen;

    @Before
    public void setUp()
    {
    writer = new StringWriter();
    commandList = new MockCommandList();
    screen = new ChatScreen()
    ...
    }

    ....
     
    markspace, Oct 30, 2010
    #8
  9. jimgardener

    Lew Guest

    On 10/30/2010 12:45 PM, markspace wrote:
    > On 10/30/2010 9:39 AM, Stanimir Stamenkov wrote:
    >
    >>> ... Relying on such default values, however, is generally
    >>> considered bad programming style.

    >>
    >> I don't completely agree with the given statement, but then I can't come
    >> up with arguments on any side. What do you think about it?


    > Lew's comment that an explicit initialization requires a second,
    > redundant and useless initialization is enough to counter Oracle's
    > statement.
    >
    > I'd say precisely the opposite. The default values are 100% guaranteed
    > and deterministic, there's no reason not to rely on them, and it's
    > certainly not bad style. I'm sorry for whoever wrote that line in the
    > tutorial but I think they went off some place unknown to most
    > professionals.


    This is a controversy. I sit somewhat in the middle - if the default initial
    value carries algorithmic significance, e.g.,

    private boolean initialized = false;

    then the redundant initialization emphasizes that the algorithm depends on
    that value, but if the usage pattern requires a call to some 'init()' or
    'setup()' or 'prepare()' or such that sets the value before it's read, or if
    otherwise there's nothing truly emphatic about the default value, then one
    should follow markspace's advice.

    private boolean initialized;

    IME a need to emphasize initialization of member references to 'null' is rarer
    than a need to emphasize initialization of primitives to their "zero-like" values.

    Mind you, it is still completely unnecessary to initialize a member twice to
    its default value just to make a point that a comment would make just as well,

    private boolean initialized; // must start out 'false'

    perhaps augmented with an invariant if you're anal retentive.

    /** Constructor. */
    public Foo()
    {
    // ... do all those lovely constructor things
    assert ! initialized;
    }

    Now that makes the point and the JVM can help enforce it.

    --
    Lew
    Anal expressive: like anal retentive but beneficially.
    The anal expressive personality pays close attention to detail but is relaxed
    about it.
     
    Lew, Oct 30, 2010
    #9
  10. jimgardener

    Lew Guest

    markspace wrote:
    > I've used instance fields but initialized them each time before a test.
    > I don't know if this is good practice or not but there it is.
    >
    > Partial code:
    >
    > public class BasicIrcParserTest
    > {
    >
    > private Writer writer;
    > private MockCommandList commandList;
    > private ChatScreen screen;
    >
    > @Before
    > public void setUp()
    > {
    > writer = new StringWriter();
    > commandList = new MockCommandList();
    > screen = new ChatScreen()
    > ...
    > }
    >
    > ...


    I aver that that is good practice. First, the default value for 'writer' will
    be 'null', so you need to initialize to the non-default values in your
    '@Before'. So you have no choice. Second, you aren't leaving it undocumented
    to those who read your test source - maintainers can readily follow the test
    flow. Third, you provide a hook for future initializations as the test class
    expands.

    --
    Lew
     
    Lew, Oct 30, 2010
    #10
  11. jimgardener

    Lew Guest

    markspace wrote:
    >> I've used instance fields but initialized them each time before a test.
    >> I don't know if this is good practice or not but there it is.
    >>
    >> Partial code:
    >>
    >> public class BasicIrcParserTest
    >> {
    >>
    >> private Writer writer;
    >> private MockCommandList commandList;
    >> private ChatScreen screen;
    >>
    >> @Before
    >> public void setUp()
    >> {
    >> writer = new StringWriter();
    >> commandList = new MockCommandList();
    >> screen = new ChatScreen()
    >> ...
    >> }
    >>
    >> ...


    Lew wrote:
    > I aver that that is good practice. First, the default value for 'writer'
    > will be 'null', so you need to initialize to the non-default values in
    > your '@Before'. So you have no choice. Second, you aren't leaving it
    > undocumented to those who read your test source - maintainers can
    > readily follow the test flow. Third, you provide a hook for future
    > initializations as the test class expands.


    Oh, and if you refer to using instance variables vs. local ones, given that by
    design JUnit creates a new test instance per test there's no risk, and it
    allows the clean separation of setup, test and teardown, so that part is good
    practice, too. Otherwise you risk setting up resources differently, or
    failing to tear down cleanly.

    Side note: Arguments an be made either way to treat the word "setup"
    (accepted by Thunderbird's spell checker) as a single word or a compound.

    --
    Lew
     
    Lew, Oct 30, 2010
    #11
  12. jimgardener

    Tom Anderson Guest

    On Sat, 30 Oct 2010, Lew wrote:

    >> public void stop(){
    >> if (isrunning) stopTime=System.currentTimeMillis();

    >
    > This 'if' block violates the coding conventions.


    Why? If you mean because it doesn't have braces round the dependent
    statement, then the coding conventions you refer to should be violated,
    because that's a perfectly fine thing to do.

    If you mean because of the lack of spaces around the equals sign, then
    fine.

    >> System.out.println("sleeping for
    >> :"+(timeinmillisecs/1000)+"secs");

    >
    > 'System.out.println()'? Really? Come on.
    >
    > If it's important enough to log, it's important enough to log correctly.
    >
    > If you really are such a schlemiel that you have to use a
    > 'System.X.println()', use 'System.err' at least. But really, use a logger.


    In a test case? No, absolutely not. There is nothing whatsoever wrong with
    using the System streams in a test case. Using a logger is pointless
    overkill.

    However, in this case, i'd suggest not logging at all, by any means,
    because the information being logged doesn't look very interesting.

    >> public void testNormalOpButForgottToStop(){
    >> watch.start();
    >> sleepForSomeTime(3000);
    >> //watch is not stopped;
    >> assertEquals(3,watch.getTotalDurationSeconds());
    >> }

    >
    > This is not a valid test. Since the watch is not stopped and sleep
    > times are approximate, you have a small risk that this assertion will
    > fail even with correct code.
    >
    > The granularity of one second will make this rare, but not theoretically
    > impossible.


    Absolutely right.

    > You should comment your code that you depend on the granularity to avoid
    > trouble.


    I'm not sure that this will avoid trouble; it'll just make it clear what
    the problem is when there is trouble.

    I'd suggest a test more like:

    public void testNormalOpButForgottToStop() {
    long startTime = System.currentTimeMillis();
    watch.start();
    sleepForSomeTime(3000);
    long endTime = System.currentTimeMillis();
    long totalDuration = watch.getTotalDurationSeconds()
    assertEquals(((endTime - startTime) / 1000), totalDuration);
    }

    tom

    --
    Ed editor textorum probatissimus est -- Cicero, De officiis IV.7
     
    Tom Anderson, Oct 30, 2010
    #12
  13. Sat, 30 Oct 2010 10:55:22 -0400, /Lew/:
    > jimgardener wrote:
    >
    >> boolean isrunning=false;

    >
    > The variable name 'isrunning' violates the naming conventions.


    Do you mean because 'running' is not capitalized as in 'isRunning'
    or because it is using 'is' as in a boolean accessor method name?

    --
    Stanimir
     
    Stanimir Stamenkov, Oct 30, 2010
    #13
  14. jimgardener

    Lew Guest

    jimgardener wrote:
    >>> public void stop(){
    >>> if (isrunning) stopTime=System.currentTimeMillis();


    Lew wrote:
    >> This 'if' block violates the coding conventions.


    Tom Anderson wrote:
    > Why? If you mean because it doesn't have braces round the dependent
    > statement, then the coding conventions you refer to should be violated,
    > because that's a perfectly fine thing to do.
    >
    > If you mean because of the lack of spaces around the equals sign, then
    > fine.


    I meant the lack of braces, something anyone familiar with the conventions
    would have spotted, and whether "that's a perfectly fine thing to do" or not
    is a separate question. That it violates the conventions is a fact.

    <http://www.oracle.com/technetwork/java/codeconventions-142311.html#449>

    --
    Lew
     
    Lew, Oct 30, 2010
    #14
  15. jimgardener

    Lew Guest

    jimgardener wrote:
    >>> boolean isrunning=false;


    Lew wrote:
    >> The variable name 'isrunning' violates the naming conventions.


    Stanimir Stamenkov wrote:
    > Do you mean because 'running' is not capitalized as in 'isRunning' or
    > because it is using 'is' as in a boolean accessor method name?


    Familiarity with the conventions makes the answer obvious.

    The former is distinctly against the conventions, as anyone familiar with the
    conventions can see,
    <http://www.oracle.com/technetwork/java/codeconventions-135099.html#367>

    I am aware of nothing in that document that forbids 'isRunning' as a boolean
    variable name, or any other type variable name for that matter.

    --
    Lew
     
    Lew, Oct 30, 2010
    #15
  16. jimgardener

    Tom Anderson Guest

    On Sat, 30 Oct 2010, Lew wrote:

    > jimgardener wrote:
    >>>> public void stop(){
    >>>> if (isrunning) stopTime=System.currentTimeMillis();

    >
    > Lew wrote:
    >>> This 'if' block violates the coding conventions.

    >
    > Tom Anderson wrote:
    >> Why? If you mean because it doesn't have braces round the dependent
    >> statement, then the coding conventions you refer to should be violated,
    >> because that's a perfectly fine thing to do.
    >>
    >> If you mean because of the lack of spaces around the equals sign, then
    >> fine.

    >
    > I meant the lack of braces, something anyone familiar with the conventions
    > would have spotted, and whether "that's a perfectly fine thing to do" or not
    > is a separate question. That it violates the conventions is a fact.


    True. I'm uncertain what the value of pointing the fact out is, though,
    when it's a rule that isn't a very good one.

    tom

    --
    The Impossible is True
     
    Tom Anderson, Oct 31, 2010
    #16
  17. jimgardener

    Tom Anderson Guest

    On Sat, 30 Oct 2010, Lew wrote:

    > markspace wrote:
    >> I've used instance fields but initialized them each time before a test.
    >> I don't know if this is good practice or not but there it is.
    >>
    >> Partial code:
    >>
    >> public class BasicIrcParserTest
    >> {
    >>
    >> private Writer writer;
    >> private MockCommandList commandList;
    >> private ChatScreen screen;
    >>
    >> @Before
    >> public void setUp()
    >> {
    >> writer = new StringWriter();
    >> commandList = new MockCommandList();
    >> screen = new ChatScreen()
    >> ...
    >> }
    >>
    >> ...

    >
    > I aver that that is good practice.


    Agreed.

    For some reason, it has become the normal style to do test setup in an
    @Before method rather than a constructor or initialisers, so that's where
    people reading tests will look for it. It's preferable to put it where
    people will look for it.

    tom

    --
    The Impossible is True
     
    Tom Anderson, Oct 31, 2010
    #17
  18. jimgardener

    markspace Guest

    On 10/31/2010 12:16 PM, Tom Anderson wrote:

    > For some reason, it has become the normal style to do test setup in an
    > @Before method rather than a constructor or initialisers, so that's
    > where people reading tests will look for it. It's preferable to put it
    > where people will look for it.



    @Before gets run before each individual test. In my test harness, that
    @Before gets run about 8 times as each test executes.

    That's the point: don't let state leak from one test to another. Set
    the test harness up fresh before each test. @Before provides a
    convenient way of doing this.
     
    markspace, Oct 31, 2010
    #18
  19. jimgardener

    Jim Janney Guest

    markspace <> writes:

    > On 10/30/2010 9:39 AM, Stanimir Stamenkov wrote:
    >
    >>> ... Relying on such default values, however, is generally
    >>> considered bad programming style.

    >>
    >> I don't completely agree with the given statement, but then I can't come
    >> up with arguments on any side. What do you think about it?

    >
    >
    > Lew's comment that an explicit initialization requires a second,
    > redundant and useless initialization is enough to counter Oracle's
    > statement.


    Premature and probably pointless micro-optimization. I'm having
    trouble imagining a class where this could make a measurable
    difference in execution speed or a meaningful difference in code size.

    First make it readable. Then make it right. Then, as an optional
    step if that turns out to be necessary, worry about making it fast.

    > I'd say precisely the opposite. The default values are 100%
    > guaranteed and deterministic, there's no reason not to rely on them,
    > and it's certainly not bad style. I'm sorry for whoever wrote that
    > line in the tutorial but I think they went off some place unknown to
    > most professionals.


    Professionals understand that readability normally trumps minor
    efficiency gains. We can debate which style is clearer, but redundant
    code shouldn't be a concern.

    --
    Jim Janney
     
    Jim Janney, Nov 1, 2010
    #19
  20. jimgardener

    Tom Anderson Guest

    On Sun, 31 Oct 2010, markspace wrote:

    > On 10/31/2010 12:16 PM, Tom Anderson wrote:
    >
    >> For some reason, it has become the normal style to do test setup in an
    >> @Before method rather than a constructor or initialisers, so that's
    >> where people reading tests will look for it. It's preferable to put it
    >> where people will look for it.

    >
    > @Before gets run before each individual test. In my test harness, that
    > @Before gets run about 8 times as each test executes.


    Right. But the constructor also gets run before each individual test,
    because each test gets a fresh instance of the object. Hence, the
    difference between @Before and the constructor is a matter of style, not
    function.

    An illustrative example:

    import org.junit.*;
    public class TestCount {
    public TestCount() {System.err.println("constructor");}
    @Before public void before() {System.err.println("before");}
    @Test public void one() {System.err.println("one");}
    @Test public void two() {System.err.println("two");}
    }

    I get:

    JUnit version 4.6
    .constructor
    before
    one
    .constructor
    before
    two

    Time: 0.032

    OK (2 tests)

    It occurs to me that there is no alternative to using @After, given that
    finalizers and weak references and so on do not execute in bounded time.
    Given that you have to use @After for teardown, symmetry favours using
    @Before for setup.

    tom

    --
    The literature is filled with bizarre occurrances for which we have
    no explanation
     
    Tom Anderson, Nov 1, 2010
    #20
    1. Advertising

Want to reply to this thread or ask your own question?

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. Chris Shenton
    Replies:
    1
    Views:
    1,485
    =?ISO-8859-1?Q?Michael_Str=F6der?=
    Aug 24, 2007
  2. eggie5

    Help with junit setup

    eggie5, Oct 8, 2007, in forum: Java
    Replies:
    8
    Views:
    5,525
    eggie5
    Oct 9, 2007
  3. Scott
    Replies:
    1
    Views:
    142
    Timothy Hunter
    Aug 20, 2005
  4. Michael Schuerig
    Replies:
    6
    Views:
    156
    Robert Dober
    Feb 26, 2007
  5. Intransition

    [ANN] Ruby Setup 5 (setup.rb)

    Intransition, Jan 13, 2010, in forum: Ruby
    Replies:
    0
    Views:
    439
    Intransition
    Jan 13, 2010
Loading...

Share This Page