1368262 - Permaorange in webdriver/navigation.py when Gecko 55 merges to beta on 2017-06-12

Status: RESOLVED WORKSFORME

(Testing :: web-platform-tests, defect)

Description

[Phil Ringnalda (:philor)]

Three permaorange unexpected passes (which have been around since the mid-May wpt update, but I didn't notice them below all of the bug 1358020 failures):

https://treeherder.mozilla.org/logviewer.html#?job_id=102582424&repo=try

TEST-UNEXPECTED-PASS | /webdriver/navigation.py | navigation.py::test_get_current_url_alert_prompt - expected FAIL
INFO - TEST-UNEXPECTED-PASS | /webdriver/navigation.py | navigation.py::test_set_malformed_url - expected FAIL
INFO - TEST-UNEXPECTED-OK | /webdriver/navigation.py | expected TIMEOUT

which is nice as a hint for the apparently-unfiled bug about expecting failure, the reason those two fail is because of something that's only enabled on nightly, but shorter range, it's only two weeks until we merge to beta and

[Tracking Requested - why for this release]: merge bustage, closed tree, delayed b1, wailing and gnashing of teeth.

Comment 1

[Julien Cristau [:jcristau]]

marking as blocker for 55.  James, is this something you can look at?

Comment 2

[James Graham [:jgraham]]

Attachment: 1368262.diff

Something like that?

Comment 3

[Mike Taylor [:miketaylr]]

James, did you mean to request review on that patch?

Comment 4

[James Graham [:jgraham]]

Sure I guess? Not sure who should review it. Previously with this kind of thing someone who knows how has done a beta simulation run with the patch applied and it has landed as a=testonly or similar. I don't know how to request that more than needinfoing a random sheriff though.

Comment 5

[Mike Taylor [:miketaylr]]

Roger that, let's ni? phil. :)

Comment 6

[Phil Ringnalda (:philor)]

Where's the dependency on a bug about adding awareness of RELEASE_OR_BETA to wpt .inis so we don't have to land things on a closed tree after the merge to unbust stuff?

Where's the dependency on a bug about investigating what thing which is enabled on trunk and disabled on RELEASE_OR_BETA causes the test to fail on trunk and pass on beta?

Comment 7

[Ryan VanderMeulen [:RyanVM]]

James, can you please reply to comment 6? FYI, this is currently marked as blocking Monday's merge of 55 to Beta.

Comment 8

[James Graham [:jgraham]]

> Where's the dependency on a bug about adding awareness of RELEASE_OR_BETA to wpt .inis so we don't have to land things on a closed tree after the merge to unbust stuff?

That's in the mozinfo.json files now? Then we can use it. In fact it probably already works, although we will never add it as metadata when updating. Anyway filed.

Why aren't we keeping a tree that's m+c + patches to run it as beta + patches required for beta specifically, and rebasing it on m-c every time we want to do a new simulation run (or just with a cron job that does that once a day). Having to apply patches after the merge seems entirely avoidable.

> Where's the dependency on a bug about investigating what thing which is enabled on trunk and disabled on RELEASE_OR_BETA causes the test to fail on trunk and pass on beta?

I think it makes more sense to ask the test author/feature owner who happens to be ato in this case.

Comment 9

[Phil Ringnalda (:philor)]

I know gps has a "correct way to use DVCS, not this horrible awful CVS-inspired hack of separate repos" plan for how branching should work, that's the only thing different than the current scheme that we'll be allowed to change to, but actually listening to the details of ocean-boiling pie-in-the-sky pipe dreams just makes me sad, so I don't know what it is or whether it will make pre-landing things on a beta (or an infinite series of future betas) that doesn't yet exist possible.

Comment 10

[Phil Ringnalda (:philor)]

Inconveniently, this patch won't work, since whatever unknown thing is failing on the trunk is also failing on DevEdition, which is currently green with the expectation that we'll fail, and with this patch would become permaorange. So the patch we need is the one which simply completely disables the test from running at all until someone wants to look at it, landed on the trunk, before Monday morning.

Comment 11

[Andreas Tolfsen ❲:ato❳]

(In reply to Phil Ringnalda (:philor) from comment #6)
> Where's the dependency on a bug about adding awareness of RELEASE_OR_BETA to
> wpt .inis so we don't have to land things on a closed tree after the merge
> to unbust stuff?
> 
> Where's the dependency on a bug about investigating what thing which is
> enabled on trunk and disabled on RELEASE_OR_BETA causes the test to fail on
> trunk and pass on beta?

I don’t understand exactly what you’re asking for here, but as far as I can
tell from jgraham’s attached patch, two tests are getting re-enabled because
they have started passing?

> -[navigation.py]
> -  type: wdspec
> -  expected: TIMEOUT
> -  [navigation.py::test_get_current_url_alert_prompt]
> -    expected: FAIL
> -
> -  [navigation.py::test_set_malformed_url]
> -    expected: FAIL

I believe the first test, test_get_current_url_alert_prompt, was fixed by
https://bugzilla.mozilla.org/show_bug.cgi?id=1359004, which correctly is only
available on 55.

The second I’m a bit more unsure about because many changes have been made in
this area, but I think it can be tracked down to
https://bugzilla.mozilla.org/show_bug.cgi?id=1335778.  Curiously, this is
already available on 54 because it was uplifted.

Comment 12

[Andreas Tolfsen ❲:ato❳]

I built m-c with official branding locally and it passes testing/web-platform/tests/webdriver/navigation.py.

Comment 13

[Andreas Tolfsen ❲:ato❳]

jgraham and I investigated this some more.  We found that when the RELEASE_OR_BETA ifdef is set, which can be caused by removing the “a” (alpha) character from config/milestone.txt, this causes sandboxed processes to get disabled.  This causes testing/web-platform/tests/webdriver/navigation.py’s file: URL test to start passing (which is an unexpected pass) because it is no longer reloading the content frame script.

With sandboxed processes enabled, content frame scripts are reloaded as Marionette navigates to a file: URL, but Marionette currently assumes that a frame script reload is always linked to when a remoteness change occurs.  There is https://bugzil.la/1332122 to fix this assumption.

Comment 14

[James Graham [:jgraham]]

Attachment: Bug 1368262 - Disable webdriver test that behaves differently on dev edition vs release,

The test times out on master and dev edition but not on normal beta. Since
this is difficult to handle in ini files just disable the entire test until there's
a proepr fix for the underlying issue.

Review commit: https://reviewboard.mozilla.org/r/147566/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/147566/

Comment 15

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8876158 [details]
Bug 1368262 - Disable webdriver test that behaves differently on dev edition vs release,

https://reviewboard.mozilla.org/r/147566/#review152152

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1a3550df4043
Disable webdriver test that behaves differently on dev edition vs release, r=ato

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/1a3550df4043

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+1]]

We should keep the bug open for the remaining work to get the test re-enabled. Just marking it disabled for 55.0.

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+1]]

At some point in the past this was fixed. There is no disable line anymore for any of the navigation tests.