Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7576024:
Provide ability for WebUIBrowserTests to run asynchronous tests. (Closed)
|
Created:
9 years, 4 months ago by Sheridan Rawlins Modified:
9 years, 4 months ago CC:
chromium-reviews, dpapad Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionProvide ability for WebUIBrowserTests to run asynchronous tests.
BUG=91737
R=mmenke@chromium.org,dtseng@chromium.org
TEST=browser_tests --gtest_filter=WebUIBrowserAsyncTest.*
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=96620
Patch Set 1 #Patch Set 2 : Added isAsyncTestDone. #Patch Set 3 : Added currentTestFunction & currentTestArguments for asyncTestDone. #Patch Set 4 : Don't need to expose isAsyncTestDone. #
Total comments: 6
Patch Set 5 : Address Mark's comments. #Patch Set 6 : corrected case testing async is done. #Patch Set 7 : Added log for quitting the message loop and quit first. #Patch Set 8 : Added some tests for async behavior. #Patch Set 9 : Added comments, and mock to ensure that test passing works. #Patch Set 10 : Added tests to verify pass-pass and pass-fail work. #
Total comments: 11
Patch Set 11 : Moved |errors| around and do errors.splice only when completing results. #
Total comments: 2
Patch Set 12 : Added test to ensure early asyncTestDone still terminates when calling WaitForAsyncResult(). #Patch Set 13 : Ditched WaitForAsyncResult - rolled async stuff into WaitForResult. #Patch Set 14 : Added failure and test when asyncTestDone() is called for non-async tests. #
Total comments: 15
Patch Set 15 : More review cleanups and comments. #Patch Set 16 : Logic to handle multiple events coming on one RunMessageLoop() call. #
Total comments: 23
Patch Set 17 : More comment/test updates. #Patch Set 18 : Some more comment updates. #Patch Set 19 : test_api always sends testResult message (by calling testDone() when !isAsync). #Patch Set 20 : Rebase. #
Total comments: 7
Patch Set 21 : Review comments, fixed chrome.send passthrough, fixed assertion tests to use runTestFunction. #Messages
Total messages: 33 (0 generated)
Mark will take this patch and amend his changes in http://codereview.chromium.org/7553009/ to use the WebUI messaging to communicate doneness rather than the title setting mechanism.
http://codereview.chromium.org/7576024/diff/3002/chrome/browser/ui/webui/web_... File chrome/browser/ui/webui/web_ui_test_handler.cc (right): http://codereview.chromium.org/7576024/diff/3002/chrome/browser/ui/webui/web_... chrome/browser/ui/webui/web_ui_test_handler.cc:40: ui_test_utils::RunMessageLoop(); You need to check |test_done_async_| here, and not enter the message loop if it's already done. http://codereview.chromium.org/7576024/diff/3002/chrome/browser/ui/webui/web_... chrome/browser/ui/webui/web_ui_test_handler.cc:52: test_done_async_ = true; Neither this nor |is_waiting_async_| is ever initialized to false. http://codereview.chromium.org/7576024/diff/3002/chrome/browser/ui/webui/web_... chrome/browser/ui/webui/web_ui_test_handler.cc:63: ASSERT_TRUE(test_result->GetBoolean(0, success)); You should probably initialize success to false, before this call, just to be paranoid.
http://codereview.chromium.org/7576024/diff/3002/chrome/browser/ui/webui/web_... File chrome/browser/ui/webui/web_ui_test_handler.cc (right): http://codereview.chromium.org/7576024/diff/3002/chrome/browser/ui/webui/web_... chrome/browser/ui/webui/web_ui_test_handler.cc:40: ui_test_utils::RunMessageLoop(); On 2011/08/05 00:47:22, Matt Menke wrote: > You need to check |test_done_async_| here, and not enter the message loop if > it's already done. Done. http://codereview.chromium.org/7576024/diff/3002/chrome/browser/ui/webui/web_... chrome/browser/ui/webui/web_ui_test_handler.cc:52: test_done_async_ = true; On 2011/08/05 00:47:22, Matt Menke wrote: > Neither this nor |is_waiting_async_| is ever initialized to false. Done. http://codereview.chromium.org/7576024/diff/3002/chrome/browser/ui/webui/web_... chrome/browser/ui/webui/web_ui_test_handler.cc:63: ASSERT_TRUE(test_result->GetBoolean(0, success)); On 2011/08/05 00:47:22, Matt Menke wrote: > You should probably initialize success to false, before this call, just to be > paranoid. Done.
FYI, I added a few tests in web_ui_browsertests.cc to demonstrate how this can be used and how synchronization & expectations can be done from C++ using gmock and chrome.send.
http://codereview.chromium.org/7576024/diff/10007/chrome/browser/ui/webui/web... File chrome/browser/ui/webui/web_ui_test_handler.cc (right): http://codereview.chromium.org/7576024/diff/10007/chrome/browser/ui/webui/web... chrome/browser/ui/webui/web_ui_test_handler.cc:46: bool WebUITestHandler::WaitForAsyncResult() { This looks virtually identical to WaitForResult. It would be cleaner if we got rid of this codepath (and it's not really async since we're waiting via a message loop). Its about as async as WaitForResult which is to say not (in the context of C++). You could just have the test call WaitForResult directly... http://codereview.chromium.org/7576024/diff/10007/chrome/browser/ui/webui/web... chrome/browser/ui/webui/web_ui_test_handler.cc:63: if (is_waiting_async_) Why do we need separate *async versions of these booleans? Is there ever a chance that WaitForResult calls WaitForAsyncResult (or vice versa)? http://codereview.chromium.org/7576024/diff/10007/chrome/browser/ui/webui/web... chrome/browser/ui/webui/web_ui_test_handler.cc:67: test_done_async_ = true; What's the difference between test_done_async and is_waiting_async? Isn't test_done_async == !is_waiting_async? And, again, why are we using dup versions of these booleans? I don't think we ever stack message loops.
http://codereview.chromium.org/7576024/diff/10007/chrome/browser/ui/webui/web... File chrome/browser/ui/webui/web_ui_test_handler.cc (right): http://codereview.chromium.org/7576024/diff/10007/chrome/browser/ui/webui/web... chrome/browser/ui/webui/web_ui_test_handler.cc:67: test_done_async_ = true; On 2011/08/05 13:45:40, David Tseng wrote: > What's the difference between test_done_async and is_waiting_async? Isn't > test_done_async == !is_waiting_async? And, again, why are we using dup versions > of these booleans? I don't think we ever stack message loops. On 2011/08/05 13:45:40, David Tseng wrote: > Why do we need separate *async versions of these booleans? Is there ever a > chance that WaitForResult calls WaitForAsyncResult (or vice versa)? The async result could theoretically be reported before the synchronous callback exits the message loop, so it needs its own bool. The need for WaitForAsyncResult follows from that. That having been said, looks like |is_waiting_async_| and |is_waiting_| could be merged, though might be a good idea to throw in an ASSERT to vertify that they're false when entering the wait functions.
On 8/5/11, mmenke@chromium.org <mmenke@chromium.org> wrote: > > http://codereview.chromium.org/7576024/diff/10007/chrome/browser/ui/webui/web... > File chrome/browser/ui/webui/web_ui_test_handler.cc (right): > > http://codereview.chromium.org/7576024/diff/10007/chrome/browser/ui/webui/web... > chrome/browser/ui/webui/web_ui_test_handler.cc:67: test_done_async_ = > true; > On 2011/08/05 13:45:40, David Tseng wrote: >> What's the difference between test_done_async and is_waiting_async? > Isn't >> test_done_async == !is_waiting_async? And, again, why are we using dup > versions >> of these booleans? I don't think we ever stack message loops. > > On 2011/08/05 13:45:40, David Tseng wrote: >> Why do we need separate *async versions of these booleans? Is there > ever a >> chance that WaitForResult calls WaitForAsyncResult (or vice versa)? > > The async result could theoretically be reported before the synchronous > callback exits the message loop, so it needs its own bool. Got it; ok. This case actually also applies for WaitForResult(); we should just merge the two methods and add a similar check in Observe().
http://codereview.chromium.org/7576024/diff/10007/chrome/browser/ui/webui/web... File chrome/browser/ui/webui/web_ui_test_handler.cc (right): http://codereview.chromium.org/7576024/diff/10007/chrome/browser/ui/webui/web... chrome/browser/ui/webui/web_ui_test_handler.cc:46: bool WebUITestHandler::WaitForAsyncResult() { The distinction is that our "normal" RunJavascriptTest runs the test and returns - that's it. Matt needed a way to let javascript register handlers and have some over/back between the handler before the test finishes. In that aspect, the C++ and the Javascript are asynchronous, and running the message loop is a pattern to wait for completion while letting messages run. Effectively both sides hook up their message handlers, but each handler needs to return in order for their respective message loops to continue. Javascript doesn't have a way to run the message loop recursively, so we needed a way to let the call to RunJavascriptTest return and the test continue somehow, and let us know when it's done via a chrome.send message. We felt that a separate wait loop was required because the setup call will return with one message, and the asyncTestDone() will return another. While it is not intended, it _is_ possible that something in the test causes asyncTestDone() to be called before the RunJavascriptTest and that method needs to store its done state independently of its waiting state to handle this out of order ("async"-like) invocation. On 2011/08/05 13:45:40, David Tseng wrote: > This looks virtually identical to WaitForResult. It would be cleaner if we got > rid of this codepath (and it's not really async since we're waiting via a > message loop). Its about as async as WaitForResult which is to say not (in the > context of C++). > > You could just have the test call WaitForResult directly... http://codereview.chromium.org/7576024/diff/10007/chrome/browser/ui/webui/web... chrome/browser/ui/webui/web_ui_test_handler.cc:63: if (is_waiting_async_) We need separate booleans because the messages are different and may be called in either order. On 2011/08/05 13:45:40, David Tseng wrote: > Why do we need separate *async versions of these booleans? Is there ever a > chance that WaitForResult calls WaitForAsyncResult (or vice versa)? http://codereview.chromium.org/7576024/diff/10007/chrome/browser/ui/webui/web... chrome/browser/ui/webui/web_ui_test_handler.cc:67: test_done_async_ = true; I like duplicating the booleans to be explicit about the messages and following the pattern seen elsewhere in various wait utilities (see ui_test_utils). Pattern for async stuff is 2 booleans (done, running) 1 return-value (result). We ditched the done variable for our WaitForResult because we knew that the only way for our callback to occur is when we were running the message loop, whereas the callback for WaitForAsyncResult could occur while we're waiting for _this_ message loop. It seemed easier to maintain if we split the two loops than trying to have the logic and corresponding calls (passing a flag/boolean all the way through RunJavascriptTest) in WaitForResult(). Afterall, the C++ side knows whether it's kicking off asynchronous stuff and can just call the wait after it's primed the test's JS handlers with RunJavascriptTest(). On 2011/08/05 13:54:18, Matt Menke wrote: > On 2011/08/05 13:45:40, David Tseng wrote: > > What's the difference between test_done_async and is_waiting_async? Isn't > > test_done_async == !is_waiting_async? And, again, why are we using dup > versions > > of these booleans? I don't think we ever stack message loops. > > On 2011/08/05 13:45:40, David Tseng wrote: > > Why do we need separate *async versions of these booleans? Is there ever a > > chance that WaitForResult calls WaitForAsyncResult (or vice versa)? > > The async result could theoretically be reported before the synchronous callback > exits the message loop, so it needs its own bool. The need for > WaitForAsyncResult follows from that. That having been said, looks like > |is_waiting_async_| and |is_waiting_| could be merged, though might be a good > idea to throw in an ASSERT to vertify that they're false when entering the wait > functions.
You should update the "TEST=" part of the CL description. http://codereview.chromium.org/7576024/diff/10007/chrome/test/data/webui/test... File chrome/test/data/webui/test_api.js (right): http://codereview.chromium.org/7576024/diff/10007/chrome/test/data/webui/test... chrome/test/data/webui/test_api.js:499: var errors = []; This should go above "function testResult()" http://codereview.chromium.org/7576024/diff/10007/chrome/test/data/webui/test... chrome/test/data/webui/test_api.js:563: errors.splice(0, errors.length); I'm still not sure about calling this here. Seems to me that it makes the most sense to clear the errors array right after results are reported, which would be at the end of runTest (Which you currently don't do), and asyncTestDone (Which you currently do).
http://codereview.chromium.org/7576024/diff/10007/chrome/test/data/webui/test... File chrome/test/data/webui/test_api.js (right): http://codereview.chromium.org/7576024/diff/10007/chrome/test/data/webui/test... chrome/test/data/webui/test_api.js:499: var errors = []; On 2011/08/05 16:08:28, Matt Menke wrote: > This should go above "function testResult()" Actually after responding to your comment below, moved above asyncTestDone(). http://codereview.chromium.org/7576024/diff/10007/chrome/test/data/webui/test... chrome/test/data/webui/test_api.js:563: errors.splice(0, errors.length); On 2011/08/05 16:08:28, Matt Menke wrote: > I'm still not sure about calling this here. Seems to me that it makes the most > sense to clear the errors array right after results are reported, which would be > at the end of runTest (Which you currently don't do), and asyncTestDone (Which > you currently do). Wow, I just had the insight that this allows the call to runTestFunction to be the "peek" we talked about. It is a peek at the current state of errors in the form of pass/fail + message. Moved the clearing to runTest and asyncTestDone, where testResults (and hence runTestFunction) just returns the current results.
I'm aware of what the goal/interaction is (basically adding back the way I used to handle test pass/fail's). What I'm _asking_ is if we can merge WaitForResult and WaitForAsyncResult because the're doing the same thing and it would be cleaner and more maintainable if they were merged. In effect, you would stop the C++ message loop if either: - we receive a notification that the javascript finished processing (as set by our Observer). - a test result is available (via the HandleTestResult code) and the result is failed. On 8/5/11, scr@chromium.org <scr@chromium.org> wrote: > > http://codereview.chromium.org/7576024/diff/10007/chrome/browser/ui/webui/web... > File chrome/browser/ui/webui/web_ui_test_handler.cc (right): > > http://codereview.chromium.org/7576024/diff/10007/chrome/browser/ui/webui/web... > chrome/browser/ui/webui/web_ui_test_handler.cc:46: bool > WebUITestHandler::WaitForAsyncResult() { > The distinction is that our "normal" RunJavascriptTest runs the test and > returns - that's it. > > Matt needed a way to let javascript register handlers and have some > over/back between the handler before the test finishes. In that aspect, > the C++ and the Javascript are asynchronous, and running the message > loop is a pattern to wait for completion while letting messages run. > Effectively both sides hook up their message handlers, but each handler > needs to return in order for their respective message loops to continue. > Javascript doesn't have a way to run the message loop recursively, so > we needed a way to let the call to RunJavascriptTest return and the test > continue somehow, and let us know when it's done via a chrome.send > message. > > We felt that a separate wait loop was required because the setup call > will return with one message, and the asyncTestDone() will return > another. While it is not intended, it _is_ possible that something in > the test causes asyncTestDone() to be called before the > RunJavascriptTest and that method needs to store its done state > independently of its waiting state to handle this out of order > ("async"-like) invocation. > > On 2011/08/05 13:45:40, David Tseng wrote: >> This looks virtually identical to WaitForResult. It would be cleaner > if we got >> rid of this codepath (and it's not really async since we're waiting > via a >> message loop). Its about as async as WaitForResult which is to say not > (in the >> context of C++). > >> You could just have the test call WaitForResult directly... > > http://codereview.chromium.org/7576024/diff/10007/chrome/browser/ui/webui/web... > chrome/browser/ui/webui/web_ui_test_handler.cc:63: if > (is_waiting_async_) > We need separate booleans because the messages are different and may be > called in either order. > > On 2011/08/05 13:45:40, David Tseng wrote: >> Why do we need separate *async versions of these booleans? Is there > ever a >> chance that WaitForResult calls WaitForAsyncResult (or vice versa)? > > http://codereview.chromium.org/7576024/diff/10007/chrome/browser/ui/webui/web... > chrome/browser/ui/webui/web_ui_test_handler.cc:67: test_done_async_ = > true; > I like duplicating the booleans to be explicit about the messages and > following the pattern seen elsewhere in various wait utilities (see > ui_test_utils). > > Pattern for async stuff is 2 booleans (done, running) 1 return-value > (result). We ditched the done variable for our WaitForResult because we > knew that the only way for our callback to occur is when we were running > the message loop, whereas the callback for WaitForAsyncResult could > occur while we're waiting for _this_ message loop. > > It seemed easier to maintain if we split the two loops than trying to > have the logic and corresponding calls (passing a flag/boolean all the > way through RunJavascriptTest) in WaitForResult(). Afterall, the C++ > side knows whether it's kicking off asynchronous stuff and can just call > the wait after it's primed the test's JS handlers with > RunJavascriptTest(). > > On 2011/08/05 13:54:18, Matt Menke wrote: >> On 2011/08/05 13:45:40, David Tseng wrote: >> > What's the difference between test_done_async and is_waiting_async? > Isn't >> > test_done_async == !is_waiting_async? And, again, why are we using > dup >> versions >> > of these booleans? I don't think we ever stack message loops. > >> On 2011/08/05 13:45:40, David Tseng wrote: >> > Why do we need separate *async versions of these booleans? Is there > ever a >> > chance that WaitForResult calls WaitForAsyncResult (or vice versa)? > >> The async result could theoretically be reported before the > synchronous callback >> exits the message loop, so it needs its own bool. The need for >> WaitForAsyncResult follows from that. That having been said, looks > like >> |is_waiting_async_| and |is_waiting_| could be merged, though might be > a good >> idea to throw in an ASSERT to vertify that they're false when entering > the wait >> functions. > > http://codereview.chromium.org/7576024/ >
On 2011/08/05 17:41:56, David Tseng wrote: > I'm aware of what the goal/interaction is (basically adding back the > way I used to handle test pass/fail's). Yep - I know you know as you wrote this first, and we had a great talk over lunch yesterday. I am trying to explain Matt's use case. Matt - maybe you can speak up on what you said about possibly needing to intersperse other C++? > What I'm _asking_ is if we can merge WaitForResult and > WaitForAsyncResult because the're doing the same thing and it would be > cleaner and more maintainable if they were merged. In effect, you > would stop the C++ message loop if either: > - we receive a notification that the javascript finished processing > (as set by our Observer). > - a test result is available (via the HandleTestResult code) and the > result is failed. Matt, you mentioned that tests you intend to write need to intersperse C++ into the mix. Can they do that with the MockHandler or do they need to exit out of the JS kickoff and do some stuff before waiting for async done? My guess is that if you don't split it, then you don't have that facility to know that your tests were kicked off (i.e. if you use RunJavascriptFunction, then you know you've sent the message, but don't know if JS has run it, but if you use RunJavascriptTest you know that your entire setup JS has run), then you can do other C++ stuff (like kicking off activity on yet another thread like IO) and WaitForResult. I'd really like to prefer the maintainability of supporting non-flaky tests over extra code, and I hope this patch/framework does support non-flaky tests.
> > I'd really like to prefer the maintainability of supporting non-flaky tests > over > extra code, and I hope this patch/framework does support non-flaky tests. I'm with you there, but I'm not sure increasing the types of wait* methods we support helps. For example, the case Matt pointed out where the javascript finishes before the C++ side even begins to wait applies equally to prematurely returning tests (WaitForAsyncResult) and full tests (WaitForResult). It's actually something we should also add to our Observe logic.
Drive-by with a testing suggestion. http://codereview.chromium.org/7576024/diff/12003/chrome/browser/ui/webui/web... File chrome/browser/ui/webui/web_ui_test_handler.cc (right): http://codereview.chromium.org/7576024/diff/12003/chrome/browser/ui/webui/web... chrome/browser/ui/webui/web_ui_test_handler.cc:15: WebUITestHandler::WebUITestHandler() Have you considered sharing code with extension tests? Looks like they have some similar facilities, and due to possible flakiness problems it'd be nice to fix any bugs just once.
On 2011/08/05 18:07:09, Sheridan Rawlins wrote:
> On 2011/08/05 17:41:56, David Tseng wrote:
> > I'm aware of what the goal/interaction is (basically adding back the
> > way I used to handle test pass/fail's).
>
> Yep - I know you know as you wrote this first, and we had a great talk over
> lunch yesterday. I am trying to explain Matt's use case. Matt - maybe you can
> speak up on what you said about possibly needing to intersperse other C++?
The three cases I'm thinking about (And there may well be more to come) are
these:
1) Send a couple fake NetLog events after starting the test.
2) Mess with some config options via C++ and make sure the page sees them.
3) Mess with some config options via Javascript and make sure the C++ sees the
changes.
3) Will need to be done with observers, so isn't relevant.
Separating out the two functions means that in cases 1) and 2) and can write a
single C++ test function, instead of having to deal with registering a bunch of
callbacks to be called from Javascript while tests are running, which just makes
life easier.
I much prefer:
WHATEVER_TEST_F(Whatever, whatever) {
// Could probably get away with RunJavascriptTest, but a little
// paranoia can't hurt.
ASSERT_TRUE(RunJavascriptTest);
// Do amazingly awesome stuff here
ASSERT_TRUE(WaitForTestCompleteAsync())
}
To:
class MyMessageHandler : public WebUIMessageHandler {
virtual void RegisterMessages() OVERRIDE {
// Register one message handler per test that needs one here.
}
// 15 other handlers for random other tests.
void AmazinglyAwesomeCode(const ListValue* list_value) {
// Do same amazingly awesome stuff here.
}
};
// 5 dozen random other tests.
// Note that though this code contains no direct references to it,
// AmazinglyAwesomeCode() will be called as a result of what the JS
// function does.
WHATEVER_TEST_F(Whatever, whatever) {
ASSERT_TRUE(RunJavascriptAsyncTest());
}
Having to put code in 3 places for every test that needs to call C++ code just
makes things painfully complicated. One disadvantage with the first approach is
that there's a theoretical initialization of about:net-internals race that I'd
also have to deal with, by some other mechanism, but I prefer a slightly more
complicated framework for the net-internals unit tests than making every single
test more complicated.
On 2011/08/05 18:14:39, David Tseng wrote: > ...I'm not sure increasing the types of wait* > methods we support helps. For example, the case Matt pointed out where > the javascript finishes before the C++ side even begins to wait > applies equally to prematurely returning tests (WaitForAsyncResult) > and full tests (WaitForResult). It's actually something we should also > add to our Observe logic. I disagree. Observe is called by receiving a message on the message loop and cannot run before we run the subloop so we are ensured of the sequence. However, if asyncTestDone() is called during the JS executed by RunJavascriptTest, then the HandleAsyncTestDone will execute during the WaitForResult message loop. Also, I'm pretty sure that Matt had a need to do more C++ after knowing that JS was executed. You'd still need to return control to the browser_test and have a way to wait again. If all of the extra stuff can be handled with messages to MockHandler, then I may reconsider this. I added another test to ensure that an early call to asyncTestDone works as expected.
http://codereview.chromium.org/7576024/diff/12003/chrome/browser/ui/webui/web... File chrome/browser/ui/webui/web_ui_test_handler.cc (right): http://codereview.chromium.org/7576024/diff/12003/chrome/browser/ui/webui/web... chrome/browser/ui/webui/web_ui_test_handler.cc:15: WebUITestHandler::WebUITestHandler() David considered this some time ago. I don't recall the full thought process, but I know that we're testing WebUI here, specifically, and at this point, we've added C++ generation, Preloading, and now async test result. I don't think that this can be merged with the extension tests at the current time (certainly not this patch) but possibly at some point would be worth considering... On 2011/08/05 18:17:14, Paweł Hajdan Jr. wrote: > Have you considered sharing code with extension tests? Looks like they have some > similar facilities, and due to possible flakiness problems it'd be nice to fix > any bugs just once.
On second thought, looking at the mock stuff, I think I can get away with just using it.
On 2011/08/05 18:59:50, Matt Menke wrote: > On second thought, looking at the mock stuff, I think I can get away with just > using it. Great - I just uploaded a patch, which ditches WaitForAsyncResult and provides RunJavascriptAsyncTest, which causes WaitForResult() to run two message loops. I actually think running twice is easier if you look at the asserts after the Quit(). The logic to determine whether or not to quit depends on code which may fail. Take a look and let me know what you think - I adjusted the tests in web_ui_browsertests.cc to use these functions and they still work.
> > I disagree. Observe is called by receiving a message on the message loop > and > cannot run before we run the subloop so we are ensured of the sequence. > However, if asyncTestDone() is called during the JS executed by > RunJavascriptTest, then the HandleAsyncTestDone will execute during the > WaitForResult message loop. > Right, but the reason why we ditched this approach earlier was because if we never receive the async message (or in earlier versions a pass/fail message) through the WebUI message handler, then we would _timeout_. Is this incorrect? That's why we went with the Observer approach. What I'm not clear on is why we are not integrating the two approaches in WaitForAsyncResult. WaitForAsyncResult can potentially timeout without setting is_waiting and quitting the message loop (as done in WaitForResult/Observe). Does that make sense? I'll try to explain in person on Monday. > Also, I'm pretty sure that Matt had a need to do more C++ after knowing > that JS > was executed. You'd still need to return control to the browser_test and > have a > way to wait again. If all of the extra stuff can be handled with messages > to > MockHandler, then I may reconsider this. > > I added another test to ensure that an early call to asyncTestDone works as > expected. > > http://codereview.chromium.org/7576024/ >
On 2011/08/06 22:29:34, David Tseng wrote: > Right, but the reason why we ditched this approach earlier was because > if we never receive the async message (or in earlier versions a > pass/fail message) through the WebUI message handler, then we would > _timeout_. Is this incorrect? That's why we went with the Observer > approach. What I'm not clear on is why we are not integrating the two > approaches in WaitForAsyncResult. WaitForAsyncResult can potentially > timeout without setting is_waiting and quitting the message loop (as > done in WaitForResult/Observe). After Mark agreed that he could use the mock-style, I implemented this in a recent patch to this CL. In writing, and discussing further use with Demetrios for Print Preview testing, I realize that a combination of both approaches is a great thing: 1) The return value, when calling Run..Test will ensure that compilation errors are caught immediately. 2) The asyncTestDone (your original WebUI message implementation) provides a mechanism to allow C++ & JS to receive messages until the test is done. I think what I'll need to implement next for Demetrios/Print Preview is a means of mocking JS global functions. I think it should be possible with the Mock4JS object and some wrapper stuff in test_api.js, and should be able to check for call and/or mark completion, as well as indicate completion. I'll probably end up moving TearDown to the asyncDone when a test is async, but that's beyond the scope of this patch, which should just provide RunJavascriptAsyncTest (C++), asyncTestDone (JS) and the logic to wait for it in WaitForResult(C++). > Does that make sense? I'll try to explain in person on Monday.
http://codereview.chromium.org/7576024/diff/15003/chrome/browser/ui/webui/web... File chrome/browser/ui/webui/web_ui_browsertest.cc (right): http://codereview.chromium.org/7576024/diff/15003/chrome/browser/ui/webui/web... chrome/browser/ui/webui/web_ui_browsertest.cc:370: EXPECT_FATAL_FAILURE(RunJavascriptTestNoReturn("FAILS_BogusFunctionName"), RunJavascriptAsyncTestNoReturn? http://codereview.chromium.org/7576024/diff/15003/chrome/browser/ui/webui/web... File chrome/browser/ui/webui/web_ui_test_handler.cc (right): http://codereview.chromium.org/7576024/diff/15003/chrome/browser/ui/webui/web... chrome/browser/ui/webui/web_ui_test_handler.cc:61: HandleTestResult(test_result, &test_succeeded_); What if an async failure is sent before a successful sync result? I suspect that won't be a common case, but you should either explicitly return an error when that happens, or set |test_succeeded_| correctly when that happens. Your code also doesn't seem to handle a sync failure and then async result, when both are received before you exit the message loop. http://codereview.chromium.org/7576024/diff/15003/chrome/test/data/webui/test... File chrome/test/data/webui/test_api.js (right): http://codereview.chromium.org/7576024/diff/15003/chrome/test/data/webui/test... chrome/test/data/webui/test_api.js:330: * Holds the errors, if any, caught by expects so that the test case can fail. nit: Add "Cleared when errors are reported to the browser process" or some such. http://codereview.chromium.org/7576024/diff/15003/chrome/test/data/webui/test... chrome/test/data/webui/test_api.js:552: * This is the guts of WebUIBrowserTest. It clears |errors|, runs the nit: It no longer clears errors. http://codereview.chromium.org/7576024/diff/15003/chrome/test/data/webui/test... chrome/test/data/webui/test_api.js:561: * @see errors nit: This should probably be moved back up to runTest.
http://codereview.chromium.org/7576024/diff/15003/chrome/browser/ui/webui/web... File chrome/browser/ui/webui/web_ui_browsertest.cc (right): http://codereview.chromium.org/7576024/diff/15003/chrome/browser/ui/webui/web... chrome/browser/ui/webui/web_ui_browsertest.cc:370: EXPECT_FATAL_FAILURE(RunJavascriptTestNoReturn("FAILS_BogusFunctionName"), On 2011/08/08 16:38:25, Matt Menke wrote: > RunJavascriptAsyncTestNoReturn? RunJavascriptTestNoReturn is not a framework method, but rather a helper function (above) read the gtest doc - it says that EXPECT_FATAL_FAILURE cannot be a member, so we have a static member set to this and use that. This is not async either. http://codereview.chromium.org/7576024/diff/15003/chrome/browser/ui/webui/web... File chrome/browser/ui/webui/web_ui_test_handler.cc (right): http://codereview.chromium.org/7576024/diff/15003/chrome/browser/ui/webui/web... chrome/browser/ui/webui/web_ui_test_handler.cc:61: HandleTestResult(test_result, &test_succeeded_); On 2011/08/08 16:38:25, Matt Menke wrote: > What if an async failure is sent before a successful sync result? I suspect > that won't be a common case, but you should either explicitly return an error > when that happens, or set |test_succeeded_| correctly when that happens. > > Your code also doesn't seem to handle a sync failure and then async result, when > both are received before you exit the message loop. Both quit the loop, so both cannot be received. If a sync failure is received, then is_waiting is set to false, and any subsequent messages don't cause the message loop to be quit. I added more comments below so the logic is explained. http://codereview.chromium.org/7576024/diff/15003/chrome/browser/ui/webui/web... chrome/browser/ui/webui/web_ui_test_handler.cc:95: if (test_succeeded_ && is_async && !test_done_async_) { I think this should be (is_async && (test_done_async_ || test_succeeded_)) So that it runs again if the test succeded (to wait for async case) or if the asyncTestDone() was called early (to wait for the sync-return. Also need to keep the previous test_succeeded to return with the correct value - if any fails then return fail. http://codereview.chromium.org/7576024/diff/15003/chrome/test/data/webui/test... File chrome/test/data/webui/test_api.js (right): http://codereview.chromium.org/7576024/diff/15003/chrome/test/data/webui/test... chrome/test/data/webui/test_api.js:330: * Holds the errors, if any, caught by expects so that the test case can fail. On 2011/08/08 16:38:25, Matt Menke wrote: > nit: Add "Cleared when errors are reported to the browser process" or some > such. Done. http://codereview.chromium.org/7576024/diff/15003/chrome/test/data/webui/test... chrome/test/data/webui/test_api.js:552: * This is the guts of WebUIBrowserTest. It clears |errors|, runs the On 2011/08/08 16:38:25, Matt Menke wrote: > nit: It no longer clears errors. Done. http://codereview.chromium.org/7576024/diff/15003/chrome/test/data/webui/test... chrome/test/data/webui/test_api.js:561: * @see errors On 2011/08/08 16:38:25, Matt Menke wrote: > nit: This should probably be moved back up to runTest. Done.
http://codereview.chromium.org/7576024/diff/15003/chrome/browser/ui/webui/web... File chrome/browser/ui/webui/web_ui_browsertest.cc (right): http://codereview.chromium.org/7576024/diff/15003/chrome/browser/ui/webui/web... chrome/browser/ui/webui/web_ui_browsertest.cc:370: EXPECT_FATAL_FAILURE(RunJavascriptTestNoReturn("FAILS_BogusFunctionName"), On 2011/08/10 01:58:53, Sheridan Rawlins wrote: > On 2011/08/08 16:38:25, Matt Menke wrote: > > RunJavascriptAsyncTestNoReturn? > > RunJavascriptTestNoReturn is not a framework method, but rather a helper > function (above) read the gtest doc - it says that EXPECT_FATAL_FAILURE cannot > be a member, so we have a static member set to this and use that. This is not > async either. In that case, you aren't using "RunJavascriptAsyncTestNoReturn" anywhere, just "RunJavascriptTestNoReturn", so might want to remove "RunJavascriptAsyncTestNoReturn" unless you intend on using it. http://codereview.chromium.org/7576024/diff/15003/chrome/browser/ui/webui/web... File chrome/browser/ui/webui/web_ui_test_handler.cc (right): http://codereview.chromium.org/7576024/diff/15003/chrome/browser/ui/webui/web... chrome/browser/ui/webui/web_ui_test_handler.cc:61: HandleTestResult(test_result, &test_succeeded_); On 2011/08/10 01:58:53, Sheridan Rawlins wrote: > On 2011/08/08 16:38:25, Matt Menke wrote: > > What if an async failure is sent before a successful sync result? I suspect > > that won't be a common case, but you should either explicitly return an error > > when that happens, or set |test_succeeded_| correctly when that happens. > > > > Your code also doesn't seem to handle a sync failure and then async result, > when > > both are received before you exit the message loop. > > Both quit the loop, so both cannot be received. If a sync failure is received, > then is_waiting is set to false, and any subsequent messages don't cause the > message loop to be quit. I added more comments below so the logic is explained. The loop does not quit instantly. It may handle currently pending tasks before it actually exits.
http://codereview.chromium.org/7576024/diff/15003/chrome/browser/ui/webui/web... File chrome/browser/ui/webui/web_ui_browsertest.cc (right): http://codereview.chromium.org/7576024/diff/15003/chrome/browser/ui/webui/web... chrome/browser/ui/webui/web_ui_browsertest.cc:370: EXPECT_FATAL_FAILURE(RunJavascriptTestNoReturn("FAILS_BogusFunctionName"), On 2011/08/10 04:17:53, Matt Menke wrote: > In that case, you aren't using "RunJavascriptAsyncTestNoReturn" anywhere, just > "RunJavascriptTestNoReturn", so might want to remove > "RunJavascriptAsyncTestNoReturn" unless you intend on using it. Thanks. When I looked at this, I didn't realize this was the 2nd flavor and had a copy/paste without rename. It actually uncovered an issue where both the EXPECT and the ASSERT were being hit (and gtest can only check for FATAL -or- NONFATAL error). Done. http://codereview.chromium.org/7576024/diff/15003/chrome/browser/ui/webui/web... File chrome/browser/ui/webui/web_ui_test_handler.cc (right): http://codereview.chromium.org/7576024/diff/15003/chrome/browser/ui/webui/web... chrome/browser/ui/webui/web_ui_test_handler.cc:61: HandleTestResult(test_result, &test_succeeded_); On 2011/08/10 04:17:53, Matt Menke wrote: > On 2011/08/10 01:58:53, Sheridan Rawlins wrote: > > On 2011/08/08 16:38:25, Matt Menke wrote: > > > What if an async failure is sent before a successful sync result? I suspect > > > that won't be a common case, but you should either explicitly return an > error > > > when that happens, or set |test_succeeded_| correctly when that happens. > > > > > > Your code also doesn't seem to handle a sync failure and then async result, > > when > > > both are received before you exit the message loop. > > > > Both quit the loop, so both cannot be received. If a sync failure is received, > > then is_waiting is set to false, and any subsequent messages don't cause the > > message loop to be quit. I added more comments below so the logic is > explained. > > The loop does not quit instantly. It may handle currently pending tasks before > it actually exits. Yup. Had to add another boolean back in... Take a look now.
Want to give it a final once over, once you've updated your comments, but looking good. http://codereview.chromium.org/7576024/diff/20001/chrome/browser/ui/webui/web... File chrome/browser/ui/webui/web_ui_browsertest.cc (right): http://codereview.chromium.org/7576024/diff/20001/chrome/browser/ui/webui/web... chrome/browser/ui/webui/web_ui_browsertest.cc:367: IN_PROC_BROWSER_TEST_F(WebUIBrowserExpectFailTest, TestFailsAsyncFast) { These tests should have brief descriptions. http://codereview.chromium.org/7576024/diff/20001/chrome/browser/ui/webui/web... chrome/browser/ui/webui/web_ui_browsertest.cc:390: // starts a failing test. nit: Capitalize "Starts" here, and just below. http://codereview.chromium.org/7576024/diff/20001/chrome/browser/ui/webui/web... chrome/browser/ui/webui/web_ui_browsertest.cc:402: nit: Remove blank line. http://codereview.chromium.org/7576024/diff/20001/chrome/browser/ui/webui/web... chrome/browser/ui/webui/web_ui_browsertest.cc:486: // Test that assertions fail immediately without calling asyncTestDone(). asyncTestDone() is, in fact, called by runAsync. http://codereview.chromium.org/7576024/diff/20001/chrome/browser/ui/webui/web... File chrome/browser/ui/webui/web_ui_test_handler.cc (right): http://codereview.chromium.org/7576024/diff/20001/chrome/browser/ui/webui/web... chrome/browser/ui/webui/web_ui_test_handler.cc:58: MessageLoopForUI::current()->Quit(); You could move this into HandleTestResult, since you duplicate the code. I'm fine with it as it is, though, just a suggestion. http://codereview.chromium.org/7576024/diff/20001/chrome/browser/ui/webui/web... chrome/browser/ui/webui/web_ui_test_handler.cc:108: // completes. This sentence isn't quite right. Maybe: When the test |is_async|, run a second message loop when not |test_done_| so that the sync test completes, or |test_succeeded_| but not |test_done_async_| so the async test completes. http://codereview.chromium.org/7576024/diff/20001/chrome/browser/ui/webui/web... chrome/browser/ui/webui/web_ui_test_handler.cc:120: // Both loops must succeed to pass. nit: If !is_async, that's not quite true. http://codereview.chromium.org/7576024/diff/20001/chrome/browser/ui/webui/web... File chrome/browser/ui/webui/web_ui_test_handler.h (right): http://codereview.chromium.org/7576024/diff/20001/chrome/browser/ui/webui/web... chrome/browser/ui/webui/web_ui_test_handler.h:46: // Populates |success| from |test_result|, logging messages to ERROR, upon nit: Second comma not needed. http://codereview.chromium.org/7576024/diff/20001/chrome/test/data/webui/test... File chrome/test/data/webui/test_api.js (right): http://codereview.chromium.org/7576024/diff/20001/chrome/test/data/webui/test... chrome/test/data/webui/test_api.js:369: // If assertion fails, the C++ backend will be immediately notified. This is not true for async tests. http://codereview.chromium.org/7576024/diff/20001/chrome/test/data/webui/test... chrome/test/data/webui/test_api.js:506: * while failing the overal test if any errors occurrred. nit: overall http://codereview.chromium.org/7576024/diff/20001/chrome/test/data/webui/test... chrome/test/data/webui/test_api.js:562: * @see createExpect nit: @see asyncTestDone?
Thanks for all of your comments - I'm glad I'm getting to share the knowledge of what the framework can do by extending use cases in this way to handle async! http://codereview.chromium.org/7576024/diff/20001/chrome/browser/ui/webui/web... File chrome/browser/ui/webui/web_ui_browsertest.cc (right): http://codereview.chromium.org/7576024/diff/20001/chrome/browser/ui/webui/web... chrome/browser/ui/webui/web_ui_browsertest.cc:367: IN_PROC_BROWSER_TEST_F(WebUIBrowserExpectFailTest, TestFailsAsyncFast) { On 2011/08/10 17:23:40, Matt Menke wrote: > These tests should have brief descriptions. Done. http://codereview.chromium.org/7576024/diff/20001/chrome/browser/ui/webui/web... chrome/browser/ui/webui/web_ui_browsertest.cc:390: // starts a failing test. On 2011/08/10 17:23:40, Matt Menke wrote: > nit: Capitalize "Starts" here, and just below. Done. http://codereview.chromium.org/7576024/diff/20001/chrome/browser/ui/webui/web... chrome/browser/ui/webui/web_ui_browsertest.cc:402: On 2011/08/10 17:23:40, Matt Menke wrote: > nit: Remove blank line. Done. http://codereview.chromium.org/7576024/diff/20001/chrome/browser/ui/webui/web... chrome/browser/ui/webui/web_ui_browsertest.cc:486: // Test that assertions fail immediately without calling asyncTestDone(). On 2011/08/10 17:23:40, Matt Menke wrote: > asyncTestDone() is, in fact, called by runAsync. Done. http://codereview.chromium.org/7576024/diff/20001/chrome/browser/ui/webui/web... File chrome/browser/ui/webui/web_ui_test_handler.cc (right): http://codereview.chromium.org/7576024/diff/20001/chrome/browser/ui/webui/web... chrome/browser/ui/webui/web_ui_test_handler.cc:58: MessageLoopForUI::current()->Quit(); There are ASSERT_* calls in Observe which occur before the call to HandleTestResult and we must Quit the messageloop regardless of early termination of result parsing code. I'm leaving this where it is, especially since you're "fine with it as is". On 2011/08/10 17:23:40, Matt Menke wrote: > You could move this into HandleTestResult, since you duplicate the code. I'm > fine with it as it is, though, just a suggestion. http://codereview.chromium.org/7576024/diff/20001/chrome/browser/ui/webui/web... chrome/browser/ui/webui/web_ui_test_handler.cc:108: // completes. On 2011/08/10 17:23:40, Matt Menke wrote: > This sentence isn't quite right. Maybe: > > When the test |is_async|, run a second message loop when not |test_done_| so > that the sync test completes, or |test_succeeded_| but not |test_done_async_| so > the async test completes. Done. http://codereview.chromium.org/7576024/diff/20001/chrome/browser/ui/webui/web... chrome/browser/ui/webui/web_ui_test_handler.cc:120: // Both loops must succeed to pass. On 2011/08/10 17:23:40, Matt Menke wrote: > nit: If !is_async, that's not quite true. Done. http://codereview.chromium.org/7576024/diff/20001/chrome/browser/ui/webui/web... File chrome/browser/ui/webui/web_ui_test_handler.h (right): http://codereview.chromium.org/7576024/diff/20001/chrome/browser/ui/webui/web... chrome/browser/ui/webui/web_ui_test_handler.h:46: // Populates |success| from |test_result|, logging messages to ERROR, upon On 2011/08/10 17:23:40, Matt Menke wrote: > nit: Second comma not needed. Done. http://codereview.chromium.org/7576024/diff/20001/chrome/test/data/webui/test... File chrome/test/data/webui/test_api.js (right): http://codereview.chromium.org/7576024/diff/20001/chrome/test/data/webui/test... chrome/test/data/webui/test_api.js:369: // If assertion fails, the C++ backend will be immediately notified. On 2011/08/10 17:23:40, Matt Menke wrote: > This is not true for async tests. Done. http://codereview.chromium.org/7576024/diff/20001/chrome/test/data/webui/test... chrome/test/data/webui/test_api.js:506: * while failing the overal test if any errors occurrred. On 2011/08/10 17:23:40, Matt Menke wrote: > nit: overall Done. http://codereview.chromium.org/7576024/diff/20001/chrome/test/data/webui/test... chrome/test/data/webui/test_api.js:562: * @see createExpect On 2011/08/10 17:23:40, Matt Menke wrote: > nit: @see asyncTestDone? Done.
LGTM. One thing you might want to think about, in this CL or another, is to have asyncTestDone always return the sole result of a test, and rename it to testDone. Then just have test_api.js call the function itself when the test entry point fails, or on sync tests. This should simplify things a little. Whether or not to do this up to you, though. http://codereview.chromium.org/7576024/diff/20001/chrome/browser/ui/webui/web... File chrome/browser/ui/webui/web_ui_test_handler.cc (right): http://codereview.chromium.org/7576024/diff/20001/chrome/browser/ui/webui/web... chrome/browser/ui/webui/web_ui_test_handler.cc:58: MessageLoopForUI::current()->Quit(); Err... Good point. On 2011/08/11 01:58:44, Sheridan Rawlins wrote: > There are ASSERT_* calls in Observe which occur before the call to > HandleTestResult and we must Quit the messageloop regardless of early > termination of result parsing code. I'm leaving this where it is, especially > since you're "fine with it as is". > > On 2011/08/10 17:23:40, Matt Menke wrote: > > You could move this into HandleTestResult, since you duplicate the code. I'm > > fine with it as it is, though, just a suggestion. >
I _really_ like that suggestion. It is a nice marriage of the WebUI chrome.send() message of the results, which David wrote, and the use of the return result to catch bogus javascript quickly (my contribution). Please take another look - I'll wait for a second signoff to commit.
On 2011/08/12 05:08:35, Sheridan Rawlins wrote: > I _really_ like that suggestion. It is a nice marriage of the WebUI > chrome.send() message of the results, which David wrote, and the use of the > return result to catch bogus javascript quickly (my contribution). FWIW, however, test_api.js cannot call something if it fails to execute. Using both messages, however made it clearer (at least to me) that one was for execution completion/success of running, and the other is for the test completion/success of testing.
LGTM http://codereview.chromium.org/7576024/diff/32003/chrome/browser/ui/webui/web... File chrome/browser/ui/webui/web_ui_test_handler.cc (right): http://codereview.chromium.org/7576024/diff/32003/chrome/browser/ui/webui/web... chrome/browser/ui/webui/web_ui_test_handler.cc:108: ui_test_utils::RunMessageLoop(); I had hoped we could get rid of this, but does seem the safest way to handle WebUITestHandler::Observe. http://codereview.chromium.org/7576024/diff/32003/chrome/browser/ui/webui/web... File chrome/browser/ui/webui/web_ui_test_handler.h (right): http://codereview.chromium.org/7576024/diff/32003/chrome/browser/ui/webui/web... chrome/browser/ui/webui/web_ui_test_handler.h:60: // Test code finished executing. nit: Might want a note that if the test is unable to execute the Javascript, still set to true.
http://codereview.chromium.org/7576024/diff/32003/chrome/browser/ui/webui/web... File chrome/browser/ui/webui/web_ui_test_handler.cc (right): http://codereview.chromium.org/7576024/diff/32003/chrome/browser/ui/webui/web... chrome/browser/ui/webui/web_ui_test_handler.cc:60: SCOPED_TRACE("WebUITestHandler::HandleAsyncTestResult"); nit: Remove Async.
Wow, the trybots uncovered a pre-existing bug with the chrome.send passthrough case to C++ (this never affected you, Matt, because you weren't using javascript handler mocks, and us, because we weren't mixing both C++ & javascript handlers). I added a comment to the line which needed a fix. Also, this shift required the assertion tests to have a facility to reset the test state (errors + testIsDone) because the assertion tests run a recursive runTestFunction which they check for the format of the errors. http://codereview.chromium.org/7576024/diff/32003/chrome/browser/ui/webui/web... File chrome/browser/ui/webui/web_ui_test_handler.cc (right): http://codereview.chromium.org/7576024/diff/32003/chrome/browser/ui/webui/web... chrome/browser/ui/webui/web_ui_test_handler.cc:60: SCOPED_TRACE("WebUITestHandler::HandleAsyncTestResult"); On 2011/08/12 14:36:44, Matt Menke wrote: > nit: Remove Async. Done. http://codereview.chromium.org/7576024/diff/32003/chrome/browser/ui/webui/web... chrome/browser/ui/webui/web_ui_test_handler.cc:108: ui_test_utils::RunMessageLoop(); On 2011/08/12 14:28:17, Matt Menke wrote: > I had hoped we could get rid of this, but does seem the safest way to handle > WebUITestHandler::Observe. Alas, this is the case given your earlier observations that we don't know if we'll get both messages in one invocation of RunMessageLoop() or two. http://codereview.chromium.org/7576024/diff/32003/chrome/browser/ui/webui/web... File chrome/browser/ui/webui/web_ui_test_handler.h (right): http://codereview.chromium.org/7576024/diff/32003/chrome/browser/ui/webui/web... chrome/browser/ui/webui/web_ui_test_handler.h:60: // Test code finished executing. On 2011/08/12 14:28:17, Matt Menke wrote: > nit: Might want a note that if the test is unable to execute the Javascript, > still set to true. Done. http://codereview.chromium.org/7576024/diff/32003/chrome/test/data/webui/test... File chrome/test/data/webui/test_api.js (right): http://codereview.chromium.org/7576024/diff/32003/chrome/test/data/webui/test... chrome/test/data/webui/test_api.js:225: this.__proto__.send.apply(this.__proto__, args); Need to include the messageName in the passthrough case. |
