Bug 784007

Summary: Server jar unit tests failing due to poorly handled test interactions
Product: [Other] RHQ Project Reporter: Jay Shaughnessy <jshaughn>
Component: TestsAssignee: Jay Shaughnessy <jshaughn>
Status: CLOSED CURRENTRELEASE QA Contact: Mike Foley <mfoley>
Severity: medium Docs Contact:
Priority: high    
Version: 4.3CC: hrupp, mazz
Target Milestone: ---   
Target Release: RHQ 4.3.0   
Hardware: All   
OS: All   
Whiteboard:
Fixed In Version: 4.3 Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2012-02-10 15:32:54 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Bug Depends On:    
Bug Blocks: 782579    

Description Jay Shaughnessy 2012-01-23 14:44:10 UTC
The addition of a new server jar test class for bug 782612 started causing
test failures generated solely due to unhandled test interactions. Meaning,
any test class run by itself would pass but running the tests together
generated errors.  

The server jar tests are fragile with respect to test invocation ordering.
testng does not guarantee test execution ordering within a test class,
nor does it guarantee that a test class Begin/testExecution/After methods
will run "atomically".

Work here aims to improve the situation, by getting the server jar tests
to again pass and hopefully be more stable going forward  It is not to re-architect the unit testing completely.

Comment 1 Jay Shaughnessy 2012-01-23 14:47:29 UTC
master commit 24d89dc93ac47178812ce63c213d176fa265e915

It seems we didn't really understand how testng was executing tests
when we submitted many test classes in a run. For example, when we
submit the server jar tests we assumed that the test classes would
be run sequentially, in perhaps some random order, and that each would
have its Before/AfterClass methods executed at the start and end of each
test class execution.

That is not true.

Although testng seems to run most tests from a test class together, it
does not seem to make that guarantee.  From latest traces it seems that
the test classes are, toa degree, pooled and:
1) you can not assume that all test methods for a test class will execute
   before execution of test methods from another test class.
2) you can not assume that multiple BeforeClass methods will not execute
   prior to an AfterClass. For example, this can happen:
         Class1:BeforeClass
         Class2:BeforeClass
         Class1:AfterClass
         Class2:AfterClass

   Put another way: You can only assume that BeforeClass and AfterClass
   execute in order, for a single test class. You can not predict the
   execution order of all Before|AfterClass methods for all test classes.

3) The same rules as described in 2 apply to Before|AfterGroups

What this means is that you must be very careful with what is done in
Before|AfterClass and BeforeAfter|Groups.  The work must not conflict
with work done in other test classes that will execute in the same test
suite.

This is very important when multiple test classes are sharing some sort
of test resource.

In general, it is much safer, although perhaps a bit slower, to use
@BeforeMethod() and @AfterMethod(alwaysRun=true).

This misunderstanding was particularly bad for us in the server jar tests,
where many test classes extend AbstractEJB3Test and utilize the jboss
mbean server under the covers, in the prepareXXX methods.  This caused
clashes described above.

So, for server jar testing this reworks things to better share the mbean
server, even among tests in the same class. And migrates all
AbstractEJB3Test.prepare/unprepare calls from Before|AfterClass|Groups to
Before|After|Method.

also - remove/suppress IDE warnings from touched test classes

Comment 2 Jay Shaughnessy 2012-01-23 14:51:24 UTC
master commit 68a688bc7bf11d2d6cfe22b08a1ffb1d282eb804

More work to prevent server jar tests from stepping on each other, or
themselves.
- tests now pass with testng 6.1.1 (current version) and also 6.3.1,
  latest version.  Each version brought out different issues due to
  different execution ordering and constraint checking.
- Avoid the use of DatabaseOperation.CLEAN_INSERT. This deletes all rows
  in tables used by the data set. This does not play well with other modules.
  Instead use DatabaseOperation.REFRESH and also clean up using
  DatabaseOperation.DELETE.  These options affect only the rows in the
  data set. (see DeleteAlertsTest)
- Unless absolutely necessary, use Before|AfterMethod for database setup
  and teardown.  The perf hit is typically minimal but the risk of
  test class conflict is high. (see DatabaseAndFilePluginDeloymentTest)
- Be more diligent to keep the database clean. For example, in addition to
  DatabaseOperation.DELETE, there may be additional data created by the
  tests. (see DiscoveryBossBeanTest)
- When possible, assertions should be narrowed to deal with only the data
  relevant to the test or test class. Don't assume the database is
  necessarily clean. This helps ensure that other test data does not cause
  failures, or is not manipulated accidentally.  Queries should try to be
  qualified with the ids known to be relevant. (see InventoryManagerBeanTest)
- Prefer to use slsb calls to do cleanup, these calls are often better at
  ensuring related data is also cleaned up. Hibernate cascading is often
  not enough. And straight SQl requires even more diligence. Cleaning up
  plugins is very important (see MetadataBeanTest).
- If a test does specific dbsetup make sure it also cleans it up, or move
  it to Before|AfterMethod. (see PluginManagerBeanTest)
- Make sure your tests can executein any order, or add specific dependencies
  to force ordering. (see ResourceMetadataManagerBean)

Other tips:
- Use unusual/varying ids for canned data.  Always starting ids at id 1 is more likely to cause conflicts.
- Always run your test class twice in a row. It should work more than once. If
  it doesn't it probably changed state of the db or other shared resource.
- Start your tests with a clean db (dbsetup) and manually inspect the
  db to see if any data was left behind (or better, write a tool to do it).

Comment 4 Charles Crouch 2012-01-23 16:11:28 UTC
Needs to be triaged to determine if needs backporting to the jon3.0.1 branch

Comment 5 John Mazzitelli 2012-02-01 20:30:58 UTC
fyi: this will replicate the problem and will run fast (you don't have to run the entire server/jar test suite):

mvn -Dtest=EventMetadataManagerBeanTest,CoreServerServiceImplTest install

Comment 6 Jay Shaughnessy 2012-02-10 15:32:54 UTC
closing, the relevant commits were backported in bug 782612.