Closed Bug 1302707 Opened 7 years ago Closed 7 years ago

Timeout error not returned when elapsing page loading timeout

Categories

(Remote Protocol :: Marionette, defect)

Product:
Remote Protocol
Component:
Marionette
Version:
Version 3
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox50 wontfix, firefox51 fixed, firefox52 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox50 --- wontfix
firefox51 --- fixed
firefox52 --- fixed

People

(Reporter: whimboo, Assigned: ato)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(14 files)

testcase
781 bytes, text/plain
Details
Bug 1302707 - Remove GeckoDriver#setScriptTimeout;
58 bytes, text/x-review-board-request
automatedtester
: review+
Details
Bug 1302707 - Remove GeckoDriver#setSearchTimeout;
58 bytes, text/x-review-board-request
automatedtester
: review+
Details
Bug 1302707 - Make GeckoDriver#timeouts spec compatible;
58 bytes, text/x-review-board-request
automatedtester
: review+
Details
Bug 1302707 - Remove Marionette.timeouts from client;
58 bytes, text/x-review-board-request
automatedtester
: review+
Details
Bug 1302707 - Add Marionette.set_page_load_timeout and adjust to new driver API;
58 bytes, text/x-review-board-request
automatedtester
: review+
Details
Bug 1302707 - Reuse Marionette.reset_timeouts in harness;
58 bytes, text/x-review-board-request
automatedtester
: review+
Details
Bug 1302707 - Correct input types for timeout function;
58 bytes, text/x-review-board-request
automatedtester
: review+
Details
Bug 1302707 - Replace uses of Marionette.timeouts with set_page_load_timeout in tests;
58 bytes, text/x-review-board-request
automatedtester
: review+
Details
Bug 1302707 - Correct Marionette tests to match API changes;
58 bytes, text/x-review-board-request
automatedtester
: review+
Details
Bug 1302707 - Add test for Marionette:timeouts compat behaviour;
58 bytes, text/x-review-board-request
automatedtester
: review+
Details
Bug 1302707 - Unignore test that expects timeout error;
58 bytes, text/x-review-board-request
automatedtester
: review+
Details
Bug 1302707 - Fix type check to allow page loading timeout of 0;
58 bytes, text/x-review-board-request
automatedtester
: review+
Details
Bug 1302707 - Set the default page loading- and script timeout;
58 bytes, text/x-review-board-request
automatedtester
: review+
Details
Attached file testcase — 
Just noticed this while investigating a hidden test failure which appeared with my changes on bug 1156427.

We seem to constantly fail to raise the TimeoutException in those cases when any about: page has been loaded before. See the attached testcase for example (part of test_navigation.py).

For now I will disable this specific test via bug 1156427. We should not forget to re-enable it.
I will take this.  hskupin caused me to open a can of worms, and I’ve found numerous other problems surrounding this.
Assignee: nobody → ato
Blocks: webdriver
Status: NEW → ASSIGNED
Comment on attachment 8791305 [details]
Bug 1302707 - Correct input types for timeout function;

https://reviewboard.mozilla.org/r/78748/#review77374

::: testing/marionette/harness/marionette/tests/unit/test_implicit_waits.py:15
(Diff revision 1)
>  class TestImplicitWaits(MarionetteTestCase):
>      def testShouldImplicitlyWaitForASingleElement(self):
>          test_html = self.marionette.absolute_url("test_dynamic.html")
>          self.marionette.navigate(test_html)
>          add = self.marionette.find_element(By.ID, "adder")
> -        self.marionette.set_search_timeout("30000")
> +        self.marionette.set_search_timeout(30000)

It might be good to leave a test with string, so we can check that the type conversion will still work.
Comment on attachment 8791306 [details]
Bug 1302707 - Replace uses of Marionette.timeouts with set_page_load_timeout in tests;

https://reviewboard.mozilla.org/r/78750/#review77376

::: testing/marionette/harness/marionette/tests/unit/test_navigation.py:128
(Diff revision 1)
>          self.assertEqual("complete", state)
>          self.assertTrue(self.marionette.find_element(By.ID, "mozLink"))
>  
>      def test_should_throw_a_timeoutexception_when_loading_page(self):
>          try:
> -            self.marionette.timeouts("page load", 0)
> +            self.marionette.set_page_load_timeout(0)

Keep in mind that this patch needs an update regarding my landed changes on autoland. The test is currently marked as skipped.
Comment on attachment 8791299 [details]
Bug 1302707 - Remove GeckoDriver#setScriptTimeout;

https://reviewboard.mozilla.org/r/78736/#review77946
Attachment #8791299 - Flags: review?(dburns) → review+
Comment on attachment 8791300 [details]
Bug 1302707 - Remove GeckoDriver#setSearchTimeout;

https://reviewboard.mozilla.org/r/78738/#review77948
Attachment #8791300 - Flags: review?(dburns) → review+
Comment on attachment 8791301 [details]
Bug 1302707 - Make GeckoDriver#timeouts spec compatible;

https://reviewboard.mozilla.org/r/78740/#review77950
Attachment #8791301 - Flags: review?(dburns) → review+
Comment on attachment 8791303 [details]
Bug 1302707 - Add Marionette.set_page_load_timeout and adjust to new driver API;

https://reviewboard.mozilla.org/r/78744/#review77952
Attachment #8791303 - Flags: review?(dburns) → review+
Comment on attachment 8791302 [details]
Bug 1302707 - Remove Marionette.timeouts from client;

https://reviewboard.mozilla.org/r/78742/#review77954
Attachment #8791302 - Flags: review?(dburns) → review+
Comment on attachment 8791304 [details]
Bug 1302707 - Reuse Marionette.reset_timeouts in harness;

https://reviewboard.mozilla.org/r/78746/#review77956
Attachment #8791304 - Flags: review?(dburns) → review+
Comment on attachment 8791305 [details]
Bug 1302707 - Correct input types for timeout function;

https://reviewboard.mozilla.org/r/78748/#review78456
Attachment #8791305 - Flags: review?(dburns) → review+
Comment on attachment 8791305 [details]
Bug 1302707 - Correct input types for timeout function;

https://reviewboard.mozilla.org/r/78748/#review77374

> It might be good to leave a test with string, so we can check that the type conversion will still work.

This is a good idea, but I think I will add this as a dedicated test case so we remember why we did it, if you agree.
Comment on attachment 8791306 [details]
Bug 1302707 - Replace uses of Marionette.timeouts with set_page_load_timeout in tests;

https://reviewboard.mozilla.org/r/78750/#review79820
Attachment #8791306 - Flags: review?(dburns) → review+
Comment on attachment 8791307 [details]
Bug 1302707 - Correct Marionette tests to match API changes;

https://reviewboard.mozilla.org/r/78752/#review79822
Attachment #8791307 - Flags: review?(dburns) → review+
Comment on attachment 8794919 [details]
Bug 1302707 - Add test for Marionette:timeouts compat behaviour;

https://reviewboard.mozilla.org/r/81142/#review79824
Attachment #8794919 - Flags: review?(dburns) → review+
Comment on attachment 8794923 [details]
Bug 1302707 - Unignore test that expects timeout error;

https://reviewboard.mozilla.org/r/81144/#review79826
Attachment #8794923 - Flags: review?(dburns) → review+
Summary: No TimeoutException raised in navigate() with timeout set after an about page was loaded → Timeout error not returned when elapsing page loading timeout
Comment on attachment 8795272 [details]
Bug 1302707 - Fix type check to allow page loading timeout of 0;

https://reviewboard.mozilla.org/r/81390/#review80248
Attachment #8795272 - Flags: review?(dburns) → review+
Comment on attachment 8795772 [details]
Bug 1302707 - Set the default page loading- and script timeout;

https://reviewboard.mozilla.org/r/81712/#review80956
Attachment #8795772 - Flags: review?(dburns) → review+
Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bcd7283f53d6
Remove GeckoDriver#setScriptTimeout; r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/b137cbc3015d
Remove GeckoDriver#setSearchTimeout; r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/9f5ec882f0b9
Make GeckoDriver#timeouts spec compatible; r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/a2e5784054a5
Remove Marionette.timeouts from client; r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/2065cb35df80
Add Marionette.set_page_load_timeout and adjust to new driver API; r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/8495bc553c45
Reuse Marionette.reset_timeouts in harness; r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/05f9545d263f
Correct input types for timeout function; r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/646af81d6c2e
Replace uses of Marionette.timeouts with set_page_load_timeout in tests; r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/f665a904fc6b
Correct Marionette tests to match API changes; r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/03a8529a7baa
Add test for Marionette:timeouts compat behaviour; r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/6d9aac56c0fa
Unignore test that expects timeout error; r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/8ef405957cfe
Fix type check to allow page loading timeout of 0; r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/f8dbc2865d67
Set the default page loading- and script timeout; r=automatedtester
Sheriffs: I’m not convinced this will apply cleanly to Aurora and Beta, but if it does please uplift as test-only changes.
Whiteboard: [checkin-needed-aurora][checkin-needed-beta]
Depends on: 1306848
As it had an serious impact on the beta release, next time, please don't bypass the process by "cheating" with a test-only approval.
Just to add here... The update tests started to fail on Oct 1st, so a day after the push to mozilla-central. That's the time when the new code hit the Nightly or test package for the first time. 

https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&filter-searchStr=fxup%20os%20x&filter-tier=1&filter-tier=2&filter-tier=3&bugfiler&fromchange=80a9c7007243ac4e931a8c4352723cbf840aff03

We should consider waiting for uplifts of Marionette changes until the next Nightly build has been successfully tested.
(In reply to Sylvestre Ledru [:sylvestre] from comment #68)
> As it had an serious impact on the beta release, next time, please don't
> bypass the process by "cheating" with a test-only approval.

Please hold your horses.  Detecting regressions caused by moving targets in out-of-tree code is impossible in the try integration process.  The changes made in these patches have no effect on end-users since they are hidden behind a flag to enable Marionette, which normally qualifies for the test-only approval.  The regression we are seeing here is in release automation, and not a regression that would affect users.

Anyway, the tl;dr here is that we are not so attached to the changes that we can’t back them out of beta.  Can I plead to someone’s good will to do that for me as I don’t have the experience doing that on a beta tree?
After discussing on IRC, we decided to backout the change:
https://hg.mozilla.org/releases/mozilla-beta/rev/5a5fc43f88e40f423cd052b2b537761a6a492da8

Then, please apologize if my tone "looked bad", I would like to restate that release management should be involved in decision when it might impact the release (automation or not). This is why I asked that the uplift approval process is used.
status-firefox50: fixed → disabled
(In reply to Sylvestre Ledru [:sylvestre] from comment #71)
> I would like to restate that
> release management should be involved in decision when it might impact the
> release (automation or not).

That is very reasonable and I apologise that I didn’t exert more diligence when writing this patch.

The core of the issue is that we have an inflexible situation with regards to making changes to the Marionette client, since it is being used for upgrade tests out-of-tree.  This means it is easy to detect regressions against the current Firefox built on try, but much harder to detect whether it continues to work reliably with older versions.

As :whimboo said on IRC, we have to use the newer Marionette client version because the upgrade tests might use new APIs that were not available in the known-compatible client with the Firefox that is being upgraded from.

Thanks for backing the patches out.
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.