Unit Test for Toggling of Encoding Auto-Detect

This is a (first) implementation for a UI test of ToggleEncodingAutoDetect(), as discussed in http://codereview.chromium.org/173265 .

While the test (seems to) work as implemented (tested both Debug and Release builds), I would like to ask the reviewers to check and comment on those parts of the test that I marked as "Hack!". In detail:

.) In order to check that disabling auto-detect doesn't result in a page load currently the test waits 500ms and checks whether a page load occurred. While this time frame should be sufficient, on heavily loaded try servers there might be cases where a page load would be triggered after 500ms in a failure case. I.e., the test isn't 100% reliable to flag such failures.
However, I haven't yet found a better, more reliable approach.


The following bullet point has been resolved:

.) [REMOVED: There are 2 other cases where I added a pause of 100ms. Without that pause the test would often hang in the following WaitForNavigation(), waiting for a navigation that has already occurred. I have to admit that I don't fully understand how this can happen (can a navigation be so fast that the recorded time stays the same?).
AFAICT those pauses are in a place where they shouldn't have another side-effect, but it's still a hackish solution.]
UPDATE: cannot reproduce the above anymore. However, there still is flakiness on shutdown, although this doesn't seem to be caused by this test per se. I filed a separate bug for this - see http://code.google.com/p/chromium/issues/detail?id=24708

Any further input would be highly appreciated.


(The patch set contains also a few remnants of the fix for the GetBooleanPreference parameter order issue, submitted at http://codereview.chromium.org/209029)

BUG=23617
TEST=BrowserEncodingTest.TestToggleAutoDetect

[Roland, 2009-10-02 06:44:08]

As previously discussed (http://codereview.chromium.org/173265), I wrote a UI
test for ToggleEncodingAutodetect. However, there are a still few unsightly
kinks where I would appreciate further input.

Cheers,

Roland

[Paweł Hajdan Jr., 2009-10-03 16:47:01]

Drive-by.

http://codereview.chromium.org/255057/diff/1/5
File chrome/browser/browser_encoding_uitest.cc (right):

http://codereview.chromium.org/255057/diff/1/5#newcode186
Line 186: WaitUntilTabCount(1);
I don't understand why this WaitUntilTabCount(1) is needed. When you get a tab
and navigate the tab, it should be there, right? Is it working around some bug
in automation?

http://codereview.chromium.org/255057/diff/1/5#newcode198
Line 198: EXPECT_TRUE(CrashAwareSleep(100));
Why is that needed?

http://codereview.chromium.org/255057/diff/1/5#newcode216
Line 216: EXPECT_TRUE(CrashAwareSleep(500));
I think that testing for "event has not happened" in this way is acceptable, at
least for now.

http://codereview.chromium.org/255057/diff/1/5#newcode227
Line 227: // TODO(rolandsteiner): HACK! Without the following sleep, the test
would
That is something that I'd be concerned about... it's going to be flaky.

http://codereview.chromium.org/255057/diff/1/2
File chrome/test/automation/automation_messages_internal.h (right):

http://codereview.chromium.org/255057/diff/1/2#newcode850
Line 850: IPC_SYNC_MESSAGE_ROUTED1_1(AutomationMsg_ToggleEncodingAutoDetect,
Please add the message at the end. Otherwise the message IDs will shift and the
reference build tests may break (same IDs will have different meaning, which
will break if tests involving the reference build use them).

[Roland, 2009-10-13 13:27:32]

Uploaded a new patch set with some minor changes and comments, see the general
description above and my inline comments below. Also refer to bug 24708.

On 2009/10/03 16:47:01, Paweł Hajdan Jr. wrote:
> Drive-by.
> 
> http://codereview.chromium.org/255057/diff/1/5#newcode186
> Line 186: WaitUntilTabCount(1);
> I don't understand why this WaitUntilTabCount(1) is needed. When you get a tab
> and navigate the tab, it should be there, right? Is it working around some bug
> in automation?

Uh, I'm no expert in this, to be honest, and just copied the boilerplate code
from the surrounding tests. I briefly tested it and removing it didn't seem to
make a difference. I left it in the new patch set, both out of a feeling of FUD,
and because this probably won't be the last revision anyway :p (see below).
 
> http://codereview.chromium.org/255057/diff/1/5#newcode216
> Line 216: EXPECT_TRUE(CrashAwareSleep(500));
> I think that testing for "event has not happened" in this way is acceptable,
at
> least for now.

Good to hear! ^_^
 
> http://codereview.chromium.org/255057/diff/1/5#newcode198
> Line 198: EXPECT_TRUE(CrashAwareSleep(100));
> Why is that needed?
[..and..] 
> http://codereview.chromium.org/255057/diff/1/5#newcode227
> Line 227: // TODO(rolandsteiner): HACK! Without the following sleep, the test
> would
> That is something that I'd be concerned about... it's going to be flaky.

Interestingly, I could not reproduce this issue anymore (even though it drove me
nuts a few weeks ago! :p) 

There unfortunately still is flakiness in this test on shutdown that I have
tried and failed to debug, only to find out that it probably isn't directly
connected with this new test per se. Rather, there seems to be some timing issue
on shutdown that is present in (at least)
BrowserEncodingTest.TestOverrideEncoding, but is masked there, as it performs
some longer file operations operations after the call to OverrideEncoding. I
filed a separate bug and discussion for this (bug 24708) and marked my own test
as FLAKY for the time being.

I'm not sure what to do about the situation though that is not a hack (e.g.,
adding a 500ms timer at the end), as so far I have been unable to find the root
cause for this.
 
> http://codereview.chromium.org/255057/diff/1/2#newcode850
> Line 850: IPC_SYNC_MESSAGE_ROUTED1_1(AutomationMsg_ToggleEncodingAutoDetect,
> Please add the message at the end. Otherwise the message IDs will shift and
the
> reference build tests may break (same IDs will have different meaning, which
> will break if tests involving the reference build use them).

Done.

[Roland Steiner, 2009-11-05 11:50:39]

After more debugging it seems the flakiness stems from the leak detector running
before everything shut down properly. I therefore explicitely close the tab
before shutdown, which AFAICT removed the flakiness.

New patch set uploaded.

[Paweł Hajdan Jr., 2009-11-05 11:56:28]

Please fix the flakiness issue before committing.

http://codereview.chromium.org/255057/diff/6001/7004
File chrome/browser/browser_encoding_uitest.cc (right):

http://codereview.chromium.org/255057/diff/6001/7004#newcode187
Line 187: scoped_refptr<TabProxy> tab(GetActiveTab());
There is tab_proxy above and now tab... it's misleading and error-prone.

http://codereview.chromium.org/255057/diff/6001/7004#newcode215
Line 215: EXPECT_TRUE(CrashAwareSleep(500));
This will be flaky. You can wait for a given number of navigations...

[Roland Steiner, 2009-11-06 04:02:47]

On 2009/11/05 11:56:28, Paweł Hajdan Jr. wrote:
> http://codereview.chromium.org/255057/diff/6001/7004#newcode187
> Line 187: scoped_refptr<TabProxy> tab(GetActiveTab());
> There is tab_proxy above and now tab... it's misleading and error-prone.

That line was actually superfluous, I must have missed it on cleanup. Removed in
the new patch set.
 
> http://codereview.chromium.org/255057/diff/6001/7004#newcode215
> Line 215: EXPECT_TRUE(CrashAwareSleep(500));
> This will be flaky. You can wait for a given number of navigations...

Now I'm confused: This (admittedly hackish) test is to verify that NO navigation
has taken place. Cf.:

On 2009/10/03 16:47:01, Paweł Hajdan Jr. wrote:
> http://codereview.chromium.org/255057/diff/1/5#newcode216
> Line 216: EXPECT_TRUE(CrashAwareSleep(500));
> I think that testing for "event has not happened" in this way is acceptable,
at
> least for now.

^_^?

[Paweł Hajdan Jr., 2009-11-06 06:12:58]

LGTM

On 2009/11/06 04:02:47, Roland Steiner wrote:
> > http://codereview.chromium.org/255057/diff/6001/7004#newcode215
> > Line 215: EXPECT_TRUE(CrashAwareSleep(500));
> > This will be flaky. You can wait for a given number of navigations...
> 
> Now I'm confused: This (admittedly hackish) test is to verify that NO
navigation
> has taken place. Cf.:
> 
> On 2009/10/03 16:47:01, Paweł Hajdan Jr. wrote:
> > http://codereview.chromium.org/255057/diff/1/5#newcode216
> > Line 216: EXPECT_TRUE(CrashAwareSleep(500));
> > I think that testing for "event has not happened" in this way is acceptable,
> at
> > least for now.
> 
> ^_^?

Yeah, you're right. I just forgot about the earlier discussion. Sorry for the
confusion.

[Johnny(Jianning) Ding, 2009-11-07 11:02:25]

On 2009/10/13 13:27:32, Roland wrote:
> Uploaded a new patch set with some minor changes and comments, see the general
> description above and my inline comments below. Also refer to bug 24708.
> 
> On 2009/10/03 16:47:01, Paweł Hajdan Jr. wrote:
> > Drive-by.
> > 
> > http://codereview.chromium.org/255057/diff/1/5#newcode186
> > Line 186: WaitUntilTabCount(1);
> > I don't understand why this WaitUntilTabCount(1) is needed. When you get a
tab
> > and navigate the tab, it should be there, right? Is it working around some
bug
> > in automation?
> 
> Uh, I'm no expert in this, to be honest, and just copied the boilerplate code
> from the surrounding tests. I briefly tested it and removing it didn't seem to
> make a difference. I left it in the new patch set, both out of a feeling of
FUD,
> and because this probably won't be the last revision anyway :p (see below).
>  
> > http://codereview.chromium.org/255057/diff/1/5#newcode216
> > Line 216: EXPECT_TRUE(CrashAwareSleep(500));
> > I think that testing for "event has not happened" in this way is acceptable,
> at
> > least for now.
> 
> Good to hear! ^_^
>  
> > http://codereview.chromium.org/255057/diff/1/5#newcode198
> > Line 198: EXPECT_TRUE(CrashAwareSleep(100));
> > Why is that needed?
> [..and..] 
> > http://codereview.chromium.org/255057/diff/1/5#newcode227
> > Line 227: // TODO(rolandsteiner): HACK! Without the following sleep, the
test
> > would
> > That is something that I'd be concerned about... it's going to be flaky.
> 
> Interestingly, I could not reproduce this issue anymore (even though it drove
me
> nuts a few weeks ago! :p) 
> 
> There unfortunately still is flakiness in this test on shutdown that I have
> tried and failed to debug, only to find out that it probably isn't directly
> connected with this new test per se. Rather, there seems to be some timing
issue
> on shutdown that is present in (at least)
> BrowserEncodingTest.TestOverrideEncoding, but is masked there, as it performs
> some longer file operations operations after the call to OverrideEncoding. I
> filed a separate bug and discussion for this (bug 24708) and marked my own
test
> as FLAKY for the time being.
> 
> I'm not sure what to do about the situation though that is not a hack (e.g.,
> adding a 500ms timer at the end), as so far I have been unable to find the
root
> cause for this.

I though this bug should be fixed by
http://src.chromium.org/viewvc/chrome?view=rev&revision=27966 (bug:23121), but
let's talk it on bug 24708.

>  
> > http://codereview.chromium.org/255057/diff/1/2#newcode850
> > Line 850: IPC_SYNC_MESSAGE_ROUTED1_1(AutomationMsg_ToggleEncodingAutoDetect,
> > Please add the message at the end. Otherwise the message IDs will shift and
> the
> > reference build tests may break (same IDs will have different meaning, which
> > will break if tests involving the reference build use them).
> 
> Done.

[Johnny(Jianning) Ding, 2009-11-07 11:09:14]

Roland,

Thanks for writing this and sorry for being late response.
One style nit, please see below comments.

http://codereview.chromium.org/255057/diff/11001/11005
File chrome/browser/browser_encoding_uitest.cc (right):

http://codereview.chromium.org/255057/diff/11001/11005#newcode175
Line 175: // turn off auto-detect before loading the file
The first character should be in uppercase in comments, and according to team
style, the comments usually end with a period. (same on line 246)