#60 Refactoring of Tests

closed-accepted
nobody
None
5
2012-09-10
2012-08-22
Hakon
No

See "State of tests" in the Development forum for context.

- Running all tests in Eclipse takes about 12 minutes for me. With this patch it takes 50 seconds. For ant test, the difference is less: From 3:30-4 minutes, to 2:15 minutes.
- While Eclipse used to find 1373 tests to run and ant test 923, both now agree there are 492 tests.

Content of patch:
- Removes all use of TestSuite. For instance, Eclipse and ant test ended up running many tests 3 times, because they were part of 2 TestSuite in addition to the TestCase.
- Renames all files containing TestCase to end with Test.java. This is standard jUnit convention. For instance some used to end with Tests.java. This makes ant test find all TestCase files.
- Reduces running time of some of the heavier tests by reducing some counts: MessengerTest.java (number of clients from 100 to 50), OddsCalculatorTest.java and especially DOddsCalculatorTest.java.

Discussion

  • Hakon

    Hakon - 2012-08-22

    svn diff output

     
  • Chris Duncan

    Chris Duncan - 2012-08-22

    You have to be careful when reducing run counts on calcing tests that it doesn't affect the results, or that the result's allowed margin is increased.
    I made a mistake earlier when I reduced the default run count for the calc, only to find tests were randomly failing (but most often succeeding) simply because the calc no longer was producing the same level of accuracy.

     
  • Hakon

    Hakon - 2012-08-22

    I tried running the tests a few times and it didn't fail once. However I could try to run the tests, say, 100 times. Will do tomorrow.

     
  • Chris Duncan

    Chris Duncan - 2012-08-22

    Your patch file did not work for me, for some reason.
    I've taken the contents and applied the renaming from Tests to Test. I also renamed the 4 suites to end in Suite instead of Test. Does that solve the issue for you?

     
  • Hakon

    Hakon - 2012-08-23

    Found the problem with the patch: svn diff doesn't support moving files, at least not for in the svn client I have (1.6). And then you would need a 1.7 svn client to properly handle that output.

    The original patch was flaky 24/100 tests. I have increased the count so it now fails 0/200, but at the expense of a little more running time. I will need to look into this in more detail.

    > Does that solve the issue for you?

    Running AllTests (in r3750) takes 1:50 minutes for me, while actually running all tests with my patch runs in 50 seconds. So I'm thinking developers should just run all tests now. There's basically little/no reason to run a large subset of the tests but not all. Removing all TestSuite also reduces the code to maintain, and may avoid some confusion as to which tests to run (and reduce breakages).

    Also, as it is now, there is no simple way to run all tests once in Eclipse that I am aware of. Ant is covered by matching against **/*Test, but I'm not aware of anything similar in Eclipse. So keeping those TestSuite still means one has to run 1.3K tests in Eclipse, basically running each test 3 times.

     
  • Hakon

    Hakon - 2012-08-24

    svn output, except for deleted files (see comment)

     
  • Hakon

    Hakon - 2012-08-24

    Eclipse currently completes AllTestsSuite in ~50 seconds, while running all tests takes ~4 minutes. With the following changes, running all tests completes in ~40 seconds.

    The change assumes we'll stop running particular test suites like AllTestsSuite, and instead run all tests. Let me know if you have any concerns about this. At least if we're thinking about the running time this patch should eliminate much of the argument of having separately maintained test suites.

    The changes consists of:
    - Modifying existing files by applying tests-refactor-try2.patch
    - Deleting all *Suite.java files

    These changes:
    - Removes all uses of TestSuite.
    - Tunes some counts to reduce the running time.

    One concern was that reducing he counts could make some tests flaky. I have run all tests 100 times, and no tests failed. The patch has been verified to apply cleanly against revision 3754.

    I found another reason to remove all public static Test suite() functions: Eclipse displays such a TestCase as junit.framework.TestSuite. Removing the suite() function makes Eclipse display the TestCase as e.g. games.strategy.engine.data.ChangeTest, which is more informative.

    Let me know what you think.

     
  • Hakon

    Hakon - 2012-08-26

    svn output

     
  • Hakon

    Hakon - 2012-08-26

    Most of tests-refactor-try2.patch has been committed. However the number of clients in the testManyClients test in MessengerTest.java got reduced to 75 instead of the suggested 25. I assume this was to make sure we're able to handle this high number of clients. This makes MessengerTest the slowest test by far.

    avoid-sleep.patch speeds up MessengerTest by avoiding an unnecessary sleep, reducing the running time by 35%, from 15 to 10 seconds on my machine. The improvement may depend on the OS - I'm using Linux.

    Verified against r3755.

     
  • Chris Duncan

    Chris Duncan - 2012-09-10
    • status: open --> closed-accepted
     
  • Chris Duncan

    Chris Duncan - 2012-09-10

    Sorry for the delay, been playing civlization.
    Patch accepted.

     

Log in to post a comment.