Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(71)

Issue 83000045: LIB Fix Flakey Unit Tests (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
11 years, 11 months ago by Brett
Modified:
11 years, 11 months ago
Reviewers:
pjo
CC:
connector-cr_google.com
Visibility:
Public.

Description

This change restores two unit tests that failed regularly when run under Windows. Both of these tests time how long it takes to shutdown either the Application or the Service, and in both cases it took slightly longer than expected. I believe the timing constraints were too tight, especially for ShutdownWaiterTest.testShutdownFullWait(), which told the waiter to wait 50 milliseconds for the shutdown to complete and complained if it took more than 58 milliseconds. This could be an issue if shutdown took many seconds, minutes, or forever, but taking 10 or 20 milliseconds longer to shutdown should not really be considered a problem. Before I disabled the test, shutdown was taking slightly more than 58 milliseconds most of the time, with an occasional breach of 60 milliseconds when run on my Windows VM. I bumped the ceiling to 65 ms and could easily be convinced that 75 or 100 ms would be acceptable. I also replace instances of Mu with microseconds in the error messages, since the character did not display correctly in the windows shell. The second flakey test, ApplicationTest.testFastShutdownWhenStarting(), I was unable to get it to fail again in 100 attempts. I even ran 2 instances of the Connector Manager coverage tests at the same time, each of which pegs the CPUs. I thought I would overheat the machine with all 4 cores pegged for nearly an hour. Therefore, I restored the test unchanged. If it starts failing again, we should address it then.

Patch Set 1 #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+6 lines, -8 lines) Patch
M test/com/google/enterprise/adaptor/ApplicationTest.java View 2 chunks +0 lines, -2 lines 0 comments Download
M test/com/google/enterprise/adaptor/ShutdownWaiterTest.java View 4 chunks +6 lines, -6 lines 2 comments Download

Messages

Total messages: 3
Brett
11 years, 11 months ago (2014-04-01 19:37:29 UTC) #1
pjo
Thank you. LGTM. I appreciate the careful CL description you've provided. https://codereview.appspot.com/83000045/diff/1/test/com/google/enterprise/adaptor/ShutdownWaiterTest.java File test/com/google/enterprise/adaptor/ShutdownWaiterTest.java (right): ...
11 years, 11 months ago (2014-04-01 19:41:38 UTC) #2
Brett
11 years, 11 months ago (2014-04-01 21:12:12 UTC) #3
Committed 01 April 2014 to Adaptor Library
To https://code.google.com/p/plexi/
   83f9672..8a614f2  master -> master

https://codereview.appspot.com/83000045/diff/1/test/com/google/enterprise/ada...
File test/com/google/enterprise/adaptor/ShutdownWaiterTest.java (right):

https://codereview.appspot.com/83000045/diff/1/test/com/google/enterprise/ada...
test/com/google/enterprise/adaptor/ShutdownWaiterTest.java:95: timeTakenUs >
48000 && timeTakenUs < 65000);
On 2014/04/01 19:41:38, pjo wrote:
> Add comment saying that an upper range of 75,000
> or 100,000 should be considered if test flakes.

Done.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b