Uploading another patch. Did not know how to do it with this message.
http://gwt-code-reviews.appspot.com/91806/diff/1/2 File user/src/com/google/gwt/junit/JUnitMessageQueue.java (right): http://gwt-code-reviews.appspot.com/91806/diff/1/2#newcode413 Line 413: assert (result != null); Good point. Fixed. On 2009/11/03 01:43:00, jlabanca wrote: > This can be null. The result is set to null to indicate that the test has > started, but no results have been received from the client. With respect to > retry, the results will be null if we retry after a remote browser dies in the > middle of a test. http://gwt-code-reviews.appspot.com/91806/diff/1/3 File user/src/com/google/gwt/junit/JUnitShell.java (right): http://gwt-code-reviews.appspot.com/91806/diff/1/3#newcode698 Line 698: getTopLogger().log(TreeLogger.ERROR, On 2009/11/02 22:09:01, jat wrote: > This formatting change doesn't look right (the previous version fit fine in 80 > characters) and is unrelated to the actual change being made. Done. http://gwt-code-reviews.appspot.com/91806/diff/1/3#newcode908 Line 908: if (batchingStrategy instanceof NoBatchingStrategy) { Done. On 2009/11/03 01:43:00, jlabanca wrote: > You should put a comment explaining why this is true. "If a BatchingStrategy is > present, the client will already have moved passed the failed test case." http://gwt-code-reviews.appspot.com/91806/diff/1/3#newcode912 Line 912: "batching does not work with retries, so not being retried"); Added a todo, will do in a separate commit On 2009/11/03 01:43:00, jlabanca wrote: > I think we should check this when we process the arguments. In fact, we should > probably add a sanity check after the args processor to verify that we don't > have conflicting args in general. http://gwt-code-reviews.appspot.com/91806/diff/1/3#newcode1005 Line 1005: prevTestInfo = currentTestInfo; Good idea. On 2009/11/03 01:43:00, jlabanca wrote: > Since runTestImpl is called recursively, can you just pass numTries as an arg? > Then you can do away with the numTries and prevTestInfo instance variables. The > static runTest methods can pass 0. http://gwt-code-reviews.appspot.com/91806/diff/1/4 File user/src/com/google/gwt/junit/RunStyle.java (right): http://gwt-code-reviews.appspot.com/91806/diff/1/4#newcode46 Line 46: public int getTries() { Have a Todo elsewhere to add a command line arg. However, I think retries is better as a RunStyle level argument than a global argument. On 2009/11/03 01:43:00, jlabanca wrote: > You should add a command line -retryCount arg to JUnitShell so the user can > specify this? I think the number of retries would have more to do with the > users test setup than with the run style. > This method should be getDefaultRetryCount, because it is the default if the > user does not specify a count. http://gwt-code-reviews.appspot.com/91806/diff/1/4#newcode78 Line 78: * an error setting up for that mode Removed On 2009/11/02 22:09:01, jat wrote: > These changes aren't related and I think are actually incorrect, since > continuation lines should be indented 4 characters. http://gwt-code-reviews.appspot.com/91806/diff/1/6 File user/test/com/google/gwt/junit/JUnitMessageQueueTest.java (right): http://gwt-code-reviews.appspot.com/91806/diff/1/6#newcode379 Line 379: int testsPerBlock = 1; On 2009/11/03 01:43:00, jlabanca wrote: > You can reuse your static values here. Done. http://gwt-code-reviews.appspot.com/91806/diff/1/7 File user/test/com/google/gwt/junit/client/GWTTestCaseTest.java (right): http://gwt-code-reviews.appspot.com/91806/diff/1/7#newcode377 Line 377: public void testRetry() { On 2009/11/03 01:43:00, jlabanca wrote: > You should add a note that this method MUST appear after testSetRetry(). Done. http://gwt-code-reviews.appspot.com/91806 --~--~---------~--~----~------------~-------~--~----~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~----------~----~----~----~------~----~------~--~---
