494905 - mochitest-browser-chrome: new browser_bug304198.js intermittently fails

Status: RESOLVED FIXED

(Firefox :: Address Bar, defect)

Description

[Serge Gautherie (:sgautherie)]

{
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1243348533.1243353640.3398.gz
Linux mozilla-central unit test on 2009/05/26 07:35:33

TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/base/content/test/browser_bug304198.js | gURLBar.value should be testPartialURL (just set) - Got http://example.org/browser/browser/base/content/test/dummy_page.html, expected http://example.org/browser/browser/base/content/test/dummy_page
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/base/content/test/browser_bug304198.js | gURLBar.value should be "" (just set) - Got http://example.org/browser/browser/base/content/test/dummy_page.html, expected
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/base/content/test/browser_bug304198.js | gURLBar.value should be testPartialURL after switching back to partialURLTab - Got http://example.org/browser/browser/base/content/test/dummy_page.html, expected http://example.org/browser/browser/base/content/test/dummy_page
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/base/content/test/browser_bug304198.js | gURLBar.value should be "" after switching back to deletedURLTab - Got http://example.org/browser/browser/base/content/test/dummy_page.html, expected
}

Comment 1

[Johnathan Nightingale [:johnath]]

sgautherie - have you seen this bug before (i.e. to know that it's intermittent, and not an actual, recent break?)

Comment 2

[Serge Gautherie (:sgautherie)]

Never seen before and previous builds with same rev did not have this bug.

Comment 3

[Dão Gottwald [:dao]]

This is basically the same as bug 477631, I think, but we probably can't use the workaround from there. Instead, we could set gBrowser.userTypedValue directly :(

Comment 4

[leftturnsolutions]

In that case would the first version of the test case be better? It is part of the 2nd patch for bug #304198

https://bug304198.bugzilla.mozilla.org/attachment.cgi?id=379572

Comment 5

[Dão Gottwald [:dao]]

That's basically what I was referring to, yes.

Comment 6

[leftturnsolutions]

Ok, I'll update the test case and send a patch in tonight.

Comment 7

[Serge Gautherie (:sgautherie)]

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1243358881.1243363339.21069.gz
Linux mozilla-central unit test on 2009/05/26 10:28:01

Comment 8

[Serge Gautherie (:sgautherie)]

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1243356358.1243363858.22132.gz
Linux mozilla-central unit test on 2009/05/26 09:45:58

Comment 9

[leftturnsolutions]

Attachment: Updated test [Backout: See comment 20]

This patch removes the EventUtils.synthesizeKey and related calls in favor of gBrowser.userTypedValue and URLBarSetURI calls so the test will pass for linux. Tested with OpenSuse 11.1.

Comment 10

[Shawn Wilsher :sdwilsh]

Are we really testing the right thing by not synthesizing key events though?  If we somehow break key hanlding, this test won't fail like it should.

Comment 11

[Shawn Wilsher :sdwilsh]

Also, do we understand /why/ synthesizeKey doesn't work?  I would rather understand the failure here and make sure we don't have a bug on the platform level.

Comment 12

[Dão Gottwald [:dao]]

(In reply to comment #10)
> If we somehow break key hanlding, this test won't fail like it should.

I'm not too concerned about that, as we have extensive human testing in that area. I'm concerned about creating tests that will fail for no good reason if we change implementation details, and therefore it would certainly be better to really simulate the user input.

(In reply to comment #11)
> Also, do we understand /why/ synthesizeKey doesn't work?

Nope, we already gave up on that in bug 477631.

Comment 13

[leftturnsolutions]

Do I need to keep the synthesizeKey approach and resolve the core issue with it not working on linux to get this test to pass? I do agree that it is the better approach, but was under the impression that I needed to go back to setting gBrowser.userTypedValue for this test...

Comment 14

[Shawn Wilsher :sdwilsh]

(In reply to comment #12)
> I'm not too concerned about that, as we have extensive human testing in that
> area. I'm concerned about creating tests that will fail for no good reason if
> we change implementation details, and therefore it would certainly be better to
> really simulate the user input.
Human testing is expensive in human time and human resources, and we should try to avoid it with automated tests whenever possible.  Machine time is cheap.


I would much prefer that we understand the issue before jumping to fixes.  I don't see how we gave up on figuring out what was wrong in bug 477631.  I don't see us getting people involved with that issue that would likely know a lot about that code (like Neil Deakin).

Comment 15

[Dão Gottwald [:dao]]

(In reply to comment #13)
> Do I need to keep the synthesizeKey approach and resolve the core issue with it
> not working on linux to get this test to pass?

That would be ideal. But first of all, you're not responsible for the
core issue. If you see a way to figure out what the core issue /is/, that would
already be a huge step forward. If you even want to resolve it, nobody will
stop you either :)

(In reply to comment #14)
> (In reply to comment #12)
> > I'm not too concerned about that, as we have extensive human testing in that
> > area. I'm concerned about creating tests that will fail for no good reason if
> > we change implementation details, and therefore it would certainly be better to
> > really simulate the user input.
> Human testing is expensive in human time and human resources, and we should try
> to avoid it with automated tests whenever possible.  Machine time is cheap.

Right, but manual testing happens constantly in this area, you don't have to ask anyone to run a litmus test or something like that. If you break typing in the location bar, you'll know as soon as anyone tries to use a build with your changeset. Apart form that, I would assume that other automated tests cover key handling already -- that's not really the point of this test. Anyway, we all agree that synthesizeKey is preferable, and I definitely would like to understand why it doesn't work reliably, as it will bite us again elsewhere.

Comment 16

[leftturnsolutions]

Ok thats fair, I'll spend on some time digging around on linux to see what the core issue is, and hopefully how to resolve it.

Comment 17

[leftturnsolutions]

I ran the browser chrome tests on linux quite a bit last night, and got intermittent failures, as reported. One thing I did notice, and this might be a) known to other besides myself, or b) just a red herring - but it appears that whenever the tests fail, a window with a single button that says "Run [test]" (browser-harness.xul?) appears in the foreground temporarily. Has anyone else seen this? Just thought I would ask those more knowledgeable in this area before going to deep down a rabbit hole.

Comment 18

[Mike Shaver (:shaver emeritus)]

Can we disable this test for now, or replace the trunk version with the test on the branch? It's preventing checkins of other blocker fixes on m-c.

Comment 19

[:Gavin Sharp [email: [e-mail]]]

I pushed the branch test to the trunk to solve the intermittent failures:
https://hg.mozilla.org/mozilla-central/rev/bfb383af1903

Comment 20

[Serge Gautherie (:sgautherie)]

(In reply to comment #19)
> https://hg.mozilla.org/mozilla-central/rev/bfb383af1903

Backed out in bug 509295.

Comment 21

[(no longer active)]

Can someone please land this on 1.9.2?

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox3.6-Unittest/1258393772.1258396872.1382.gz

Comment 22

[(no longer active)]

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox3.6-Unittest/1258561640.1258565037.8960.gz

Comment 23

[(no longer active)]

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox3.6-Unittest/1258567547.1258570008.2996.gz

Comment 24

[Serge Gautherie (:sgautherie)]

(In reply to comment #21)
> Can someone please land this on 1.9.2?

Which "this" exactly?