Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(51)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 6010005: Fix problems with my previous patch (r70105) with gmock. (Closed)

Created:
10 years ago by jam
Modified:
9 years, 7 months ago
Reviewers:
Jói
CC:
chromium-reviews, Aaron Boodman, Erik does not do reviews, pam+watch_chromium.org, Paweł Hajdan Jr.
Visibility:
Public.

Description

Fix problems with my previous patch (r70105) with gmock. TBR=joi Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=70138

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+3 lines, -3 lines) Patch
M chrome/browser/extensions/extension_uitest.cc View 3 chunks +3 lines, -3 lines 0 comments Download

Messages

Total messages: 4 (0 generated)
jam
10 years ago (2010-12-24 04:34:15 UTC) #1
jam
hey Joi, this gets the tree green because the tests stop crashing. However, the tests ...
10 years ago (2010-12-24 05:37:38 UTC) #2
Jói
Hi John, it's a holiday for me today but if you'd like to mark the ...
10 years ago (2010-12-24 13:49:57 UTC) #3
jam
9 years, 12 months ago (2010-12-26 22:00:11 UTC) #4 
Hey Joi, the tests were already marked as flaky, so they're not turning the
tree red.

I had updated the mock functions, but the gmock code was hard to follow for
someone not familiar with gmock..  my best guess at the end was that the
test code depended on the mock function having the same number of parameters
as another one (i.e. in ExternalTabUITestMockClient, OnRequestStart and
ReplyStarted).  I changed OnRequestStart to have less parameters, but the
other function needed to remain the same.  This is my guess though.

This brings up the question of gmock in testing.  Using them means that not
just whoever wrote them needs to be familiar with gmock, but anyone else who
might have to touch the non-test code.  An example of how dense this code
can be is:

 1049        EXPECT_CALL(*mock_, OnRequestStart(1, 2, testing::AllOf(
 1050           testing::Field(&IPC::AutomationURLRequest::url, StrEq(url +
"/")),
 1051           testing::Field(&IPC::AutomationURLRequest::method,
StrEq("GET")))))
 1052       .Times(1)
 1053       // We can simply do CreateFunctor(1, 2, &http_200) since we know
the
 1054       // tab handle and request id, but using WithArgs<> is much more
fancy :)
 1055       .WillOnce(testing::WithArgs<0,
1>(testing::Invoke(CreateFunctor(mock_,
 1056           &ExternalTabUITestMockClient::ReplyStarted,
 1057           &ExternalTabUITestMockClient::http_200))));

In the case of this cleanup, I had to spend more time getting the gmock code
compiling, and then figuring out why they're failing (and in the end giving
up), than the actual cleanup.  It's kind of unfortunate that testing code
itself will be an obstacle to cleanup of the actual production code.

On Fri, Dec 24, 2010 at 5:49 AM, Jói Sigurðsson <joi@chromium.org> wrote:

> Hi John, it's a holiday for me today but if you'd like to mark the test
> FAILS_ and assign me the bug that would be fine.
>
> At a glance, I wonder if you updated the parameters of the mock function,
> and the test expectations, to match the change in parameters? It's a bit
> hard to read the errors on my Android but the first one where a field match
> is done seems suspicious. The others where tab ID is 2 instead of 1 are
> weird, did anything change really tab creation?
>
> Cheers & happy holidays,
> Jói
>
> (pardon brevity - sent from mobile)
> On Dec 24, 2010 5:37 AM, "John Abd-El-Malek" <jam@chromium.org> wrote:
> > hey Joi, this gets the tree green because the tests stop crashing.
> However,
> > the tests now fail all the time (they were flaky before). They spew out a
> > lot of lines related to gmock, which I can't figure out how to fix since
> I
> > haven't used gmock before. An example is below. I had changed
> > OnRequestStart to drop a parameter out of it. Any idea?
> >
> > Thanks
> >
> > [ RUN ] ExternalTabUITestPopupEnabled.FLAKY_UserGestureTargetBlank
> > unknown file: error:
> > Unexpected mock function call - returning directly.
> > Function call: OnRequestStart(2, @0012F714 140-byte object <00-00
> > 00-00 CC-CC CC-CC 60-A6 6C-09 CC-CC CC-CC CC-CC CC-CC CC-CC CC-CC
> > 15-00 00-00 1F-00 00-00 00-00 00-00 CC-CC CC-CC 47-45 54-00 CC-CC
> > CC-CC CC-CC CC-CC CC-CC CC-CC 03-00 00-00 0F-00 00-00 ... CC-CC CC-CC
> > CC-CC CC-CC CC-CC CC-CC 00-00 00-00 0F-00 00-00 00-00 00-00 CC-CC
> > CC-CC 40-CE 64-09 CC-CC CC-CC CC-CC CC-CC CC-CC CC-CC B7-00 00-00
> > BF-00 00-00 00-00 00-00 00-00 00-00 80-00 11-00>)
> > Google Mock tried the following 2 expectations, but none matched:
> >
> > .\test\automation\automation_proxy_uitest.cc:726: tried expectation
> > #0: EXPECT_CALL(*this, OnRequestStart(_, testing::AllOf(
> > testing::Field(&AutomationURLRequest::url,
> > testing::EndsWith("favicon.ico")),
> > testing::Field(&AutomationURLRequest::method, StrEq("GET")))))...
> > Expected arg #1: (is an object whose given field ends with
> > "favicon.ico") and (is an object whose given field is equal to "GET")
> > Actual: 140-byte object <00-00 00-00 CC-CC CC-CC 60-A6
> > 6C-09 CC-CC CC-CC CC-CC CC-CC CC-CC CC-CC 15-00 00-00 1F-00 00-00
> > 00-00 00-00 CC-CC CC-CC 47-45 54-00 CC-CC CC-CC CC-CC CC-CC CC-CC
> > CC-CC 03-00 00-00 0F-00 00-00 ... CC-CC CC-CC CC-CC CC-CC CC-CC CC-CC
> > 00-00 00-00 0F-00 00-00 00-00 00-00 CC-CC CC-CC 40-CE 64-09 CC-CC
> > CC-CC CC-CC CC-CC CC-CC CC-CC B7-00 00-00 BF-00 00-00 00-00 00-00
> > 00-00 00-00 80-00 11-00>, whose given field is "http://placetogo.com/"
> > Expected: to be called any number of times
> > Actual: never called - satisfied and active
> > .\test\automation\automation_proxy_uitest.cc:708: tried expectation
> > #1: EXPECT_CALL(*this, OnRequestStart(tab_handle, testing::AllOf(
> > testing::Field(&AutomationURLRequest::url, url.spec()),
> > testing::Field(&AutomationURLRequest::method, StrEq("GET")))))...
> > Expected arg #0: is equal to 1
> > Actual: 2
> > Expected: to be called once
> > Actual: never called - unsatisfied and active
> > unknown file: error: Unexpected mock function call - returning directly.
> > Function call: HandleClosed(1)
> > Google Mock tried the following 1 expectation, but it didn't match:
> >
> > .\test\automation\automation_proxy_uitest.cc:1363: EXPECT_CALL(*mock_,
> > HandleClosed(1))...
> > Expected: all pre-requisites are satisfied
> > Actual: the following immediate pre-requisites are not satisfied:
> > .\test\automation\automation_proxy_uitest.cc:1356: pre-requisite #0
> > (end of pre-requisites)
> > Expected: to be called once
> > Actual: never called - unsatisfied and active
> > .\test\automation\automation_proxy_uitest.cc(1352): error: Actual
> > function call count doesn't match EXPECT_CALL(*mock_, OnLoad(_))...
> > Expected: to be called once
> > Actual: never called - unsatisfied and active
> > .\test\automation\automation_proxy_uitest.cc(1356): error: Actual
> > function call count doesn't match EXPECT_CALL(*mock_,
> > OnAttachExternalTab(_))...
> > Expected: to be called once
> > Actual: never called - unsatisfied and active
> > .\test\automation\automation_proxy_uitest.cc(1363): error: Actual
> > function call count doesn't match EXPECT_CALL(*mock_,
> > HandleClosed(1))...
> > Expected: to be called once
> > Actual: never called - unsatisfied and active
> > .\test\automation\automation_proxy_uitest.cc(713): error: Actual
> > function call count doesn't match EXPECT_CALL(*this,
> > OnRequestRead(tab_handle, testing::Gt(0)))...
> > Expected: to be called twice
> > Actual: never called - unsatisfied and active
> > .\test\automation\automation_proxy_uitest.cc(708): error: Actual
> > function call count doesn't match EXPECT_CALL(*this,
> > OnRequestStart(tab_handle, testing::AllOf(
> > testing::Field(&AutomationURLRequest::url, url.spec()),
> > testing::Field(&AutomationURLRequest::method, StrEq("GET")))))...
> > Expected: to be called once
> > Actual: never called - unsatisfied and active
> > [ FAILED ] ExternalTabUITestPopupEnabled.FLAKY_UserGestureTargetBlank
> > (16655 ms)
> >
> >
> > On Thu, Dec 23, 2010 at 8:34 PM, <jam@chromium.org> wrote:
> >
> >> Reviewers: Jói,
> >>
> >> Description:
> >> Fix problems with my previous patch (r70105) with gmock.
> >>
> >> TBR=joi
> >>
> >>
> >> Please review this at http://codereview.chromium.org/6010005/
> >>
> >> SVN Base: svn://chrome-svn/chrome/trunk/src/
> >>
> >> Affected files:
> >> M chrome/browser/extensions/extension_uitest.cc
> >>
> >>
> >> Index: chrome/browser/extensions/extension_uitest.cc
> >> ===================================================================
> >> --- chrome/browser/extensions/extension_uitest.cc (revision 70128)
> >> +++ chrome/browser/extensions/extension_uitest.cc (working copy)
> >> @@ -134,7 +134,7 @@
> >> EXPECT_CALL(*mock_, OnForwardMessageToExternalHost(
> >> _, keys::kAutomationOrigin, keys::kAutomationRequestTarget))
> >> .WillOnce(DoAll(
> >> - SaveArg<1>(&message_received),
> >> + SaveArg<0>(&message_received),
> >> InvokeWithoutArgs(
> >> CreateFunctor(&loop_, &TimedMessageLoopRunner::Quit))));
> >>
> >> @@ -285,7 +285,7 @@
> >> EXPECT_CALL(*mock_, OnForwardMessageToExternalHost(
> >> _, keys::kAutomationOrigin, keys::kAutomationRequestTarget))
> >> .Times(2)
> >> - .WillRepeatedly(WithArgs<1>(Invoke(
> >> + .WillRepeatedly(WithArgs<0>(Invoke(
> >> CreateFunctor(this,
> >> &ExtensionTestRoundtripApiCall::CheckAndSendResponse))));
> >>
> >> @@ -472,7 +472,7 @@
> >>
> >> EXPECT_CALL(*mock_, OnForwardMessageToExternalHost(
> >> _, keys::kAutomationOrigin, _))
> >> - .WillRepeatedly(WithArgs<1, 2>(Invoke(
> >> + .WillRepeatedly(WithArgs<0, 2>(Invoke(
> >> CreateFunctor(this,
> >> &ExtensionTestBrowserEvents::HandleMessageFromChrome))));
> >>
> >>
> >>
> >>
>

Powered by Google App Engine
This is Rietveld 408576698