Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 335653 - Optimize unit tests w.r.t. store setup/teardown
Summary: Optimize unit tests w.r.t. store setup/teardown
Status: CLOSED FIXED
Alias: None
Product: EMF
Classification: Modeling
Component: cdo.releng (show other bugs)
Version: 4.0   Edit
Hardware: All All
: P3 enhancement (vote)
Target Milestone: ---   Edit
Assignee: Caspar D. CLA
QA Contact: Eike Stepper CLA
URL:
Whiteboard:
Keywords:
Depends on: 337180
Blocks:
  Show dependency tree
 
Reported: 2011-01-28 02:02 EST by Caspar D. CLA
Modified: 2011-06-23 03:41 EDT (History)
3 users (show)

See Also:
stepper: review+
caspar_d: review? (stepper)


Attachments
Patch v1 (53.27 KB, patch)
2011-02-17 06:40 EST, Caspar D. CLA
no flags Details | Diff
Patch v2 (73.45 KB, patch)
2011-02-25 06:56 EST, Caspar D. CLA
no flags Details | Diff
Patch v3 (69.07 KB, patch)
2011-02-28 03:54 EST, Eike Stepper CLA
no flags Details | Diff
Patch to detect test cases without getResourcePath() (7.27 KB, patch)
2011-04-19 05:22 EDT, Eike Stepper CLA
no flags Details | Diff
Patch to detect test cases without getResourcePath, v2 (15.20 KB, patch)
2011-04-25 07:31 EDT, Caspar D. CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Caspar D. CLA 2011-01-28 02:02:52 EST
Currently we construct a clean store for every unit test,
and tear it down after every test.

Especially when using a DB store, this makes execution of
the entire test suite pretty slow. Personally, I find this
a serious nuisance.

Most of the setup/teardown work is unnecessary for most
testcases, because they re-use the same test packages,
which means the same tables.

I think if we (me?) spend a bit of time on this, there
should be ways of drastically reducing the setup/teardown
overhead.

A few ideas:

- Having tests declare whether they need a clean store or not,
  e.g. boolean AbstractCDOTest.needsCleanStore(), or more
  finegrained control, like mustDropData(), mustDropPackages().

- Generating unique resource names in tests, instead of using
  hardcoded "/res1" in pretty much every test. This would
  isolate the test resource contents.
  
Just brainstorming..
Comment 1 Eike Stepper CLA 2011-01-28 02:36:52 EST
(In reply to comment #0)
> Especially when using a DB store, this makes execution of
> the entire test suite pretty slow. Personally, I find this
> a serious nuisance.

Agreed.

> I think if we (me?) spend a bit of time on this, there
> should be ways of drastically reducing the setup/teardown
> overhead.

That wold be great! I've assigned this bug to you ;-)

> - Having tests declare whether they need a clean store or not,
>   e.g. boolean AbstractCDOTest.needsCleanStore(), or more
>   finegrained control, like mustDropData(), mustDropPackages().

What about method-level annotations?

> - Generating unique resource names in tests, instead of using
>   hardcoded "/res1" in pretty much every test. This would
>   isolate the test resource contents.

I'm not sure if/how that could work without modifying each test case. If we have to touch each of them I'd prefer to either adjust the resource paths by hand or use the above drop mechanism.

In any case we seem to introduce some potential for unpredictability. How can a test case decide on its own whether it's safe reuse *some* former state or not?
Comment 2 Eike Stepper CLA 2011-02-06 16:35:48 EST
I think we can never be sure that a given test does not depend on *any* previous state. For one given execution order we could probably say whether a test depends on this particular previous test or not, but if the order changes you know nothing again.

What about this, we start each test case without cleaning the previous state. If it fails, we clean the previous state, execute it again and only if it fails again, we propagate the failure? If we log the retries separately we may get a picture of

1) which tests should be executed "clean" without wasting time for a dirty try. Annotations could be added to these tests to force a clean state
2) whether it's worth it at all ;-)
Comment 3 Caspar D. CLA 2011-02-16 06:29:07 EST
(In reply to comment #2)

> Annotations could be added to these tests to force a clean state

I'm moving ahead with the method annotations. Looks good so far,
will probably have a patch annotating all tests that need it, by
tomorrow.
Comment 4 Caspar D. CLA 2011-02-17 05:08:49 EST
Did someone set Component=cdo.releng intentionally here?
Or did I accidentally set it that way when I opened it?
Makes no sense to me.
Comment 5 Caspar D. CLA 2011-02-17 06:40:27 EST
Created attachment 189167 [details]
Patch v1
Comment 6 Caspar D. CLA 2011-02-25 06:56:45 EST
Created attachment 189786 [details]
Patch v2
Comment 7 Eike Stepper CLA 2011-02-28 03:42:17 EST
BTW. I found that my anti virus software consumes considerable resources, too, because it watches the system temp folder. I've added the ability to override the temp folder with a file named "org.eclipse.net4j.util.io.tmpdir" in the user.home folder. It may specify "path=/alternative/temp/folder" to place the temp folder somewhere outside the virus scanner's scope. In addition I turned off tracing in the H2 suites. I have the feeling the results are already better. In seconds:

419 branching
482 branching, range-based 
542 branching, range-based, copyOnBranch 
688 audit, range-based
694 audit
612 non-audit
Comment 8 Eike Stepper CLA 2011-02-28 03:54:22 EST
Created attachment 189916 [details]
Patch v3

I'm not the expert with the DStore tests but it seems that there's more than just drop logic in AllTestsDBHsqldb.Hsqldb.deactivateRepositories():

        if (USE_VERIFIER)
        {
          IRepository testRepository = getRepository(REPOSITORY_NAME);
          if (testRepository != null)
          {
            getVerifier(testRepository).verify();
          }
        }

I suspect that should be executed on each tearDown...
Comment 9 Caspar D. CLA 2011-02-28 03:56:50 EST
(In reply to comment #7)
> BTW. I found that my anti virus software consumes considerable resources, too,
> because it watches the system temp folder. 

That's a good tip for Windows users, but I'm not one of them. My platform
doesn't require one, hehehe.

> In addition I turned off tracing in the H2 suites.

I have tracing turned off always. See my branches/caspar/options branch in our
SVN, to which I keep my .options files switched.

Point is: the improvements I mentioned on Skype the other day, were already
free of the influences of virus scanners & tracing.
Comment 10 Eike Stepper CLA 2011-02-28 04:00:34 EST
(In reply to comment #9)
> That's a good tip for Windows users, but I'm not one of them. My platform
> doesn't require one, hehehe.

Yeah, I heard of some operating systems that are unattractive enough :P
Comment 11 Eike Stepper CLA 2011-02-28 04:01:50 EST
Please fix the Verifier issue and commit.
Comment 12 Caspar D. CLA 2011-02-28 05:31:00 EST
Committed to trunk, rev. 7305
Comment 13 Eike Stepper CLA 2011-04-19 05:22:35 EDT
Created attachment 193559 [details]
Patch to detect test cases without getResourcePath()
Comment 14 Eike Stepper CLA 2011-04-19 05:23:10 EDT
Caspar, please decide whether we want this. Around 35 test cases may need to be adjusted...
Comment 15 Caspar D. CLA 2011-04-25 07:23:39 EDT
Dude, we TOTALLY want this :-D

But the check should make an exception for testcases that declare
@NeedsCleanRepo, since they will work with a clean repo and therefore
have no need to isolate their data into a test-specific resource. 
(And not only no need; also, no way -- otherwise they shouldn't be
declaring @NeedsCleanRepo.)

Patch coming up.
Comment 16 Caspar D. CLA 2011-04-25 07:31:04 EDT
Created attachment 193983 [details]
Patch to detect test cases without getResourcePath, v2
Comment 17 Caspar D. CLA 2011-04-25 07:32:00 EDT
(In reply to comment #16)
> Created attachment 193983 [details]
> Patch to detect test cases without getResourcePath, v2

BTW, patch includes fixes for a few testcases, either making them
use getResourcePath or declare @NeedsCleanRepo.
Comment 18 Eike Stepper CLA 2011-04-26 03:52:16 EDT
Committed revision 7637:
- trunk/plugins/org.eclipse.emf.cdo.tests
Comment 19 Eike Stepper CLA 2011-04-26 03:52:51 EDT
Thanks, seems to work perfect now!
Comment 20 Eike Stepper CLA 2011-06-23 03:41:23 EDT
Available in R20110608-1407