When using the mozmill.utils.waitFor method, the timeout is not kept alive once the condition is met. The controller.waitFor method sends a pass message, however we cannot use this method when we don't have a reference to a controller, or are closing application windows.

Pointer to Github pull-request

Comment on attachment 666585 [details] Pointer to Github pull request: https://github.com/mozilla/mozmill/pull/106 This looks fine for Mozmill 1.5. I'm not sure if we can do such a thing for Mozmill 2.0 which also would need this fix. For that branch I would vote for moving the sleep() and waitFor() method from utils.js to mozmill.js. That way we can make use of the broker to send messages to the framework.

Raising severity here because it's causing an application disconnect and drastically affects the whole testrun.

(In reply to Henrik Skupin (:whimboo) from comment #2) > This looks fine for Mozmill 1.5. I'm not sure if we can do such a thing for > Mozmill 2.0 which also would need this fix. For that branch I would vote for > moving the sleep() and waitFor() method from utils.js to mozmill.js. That > way we can make use of the broker to send messages to the framework. Andrew, do you have any problems with that proposal for Mozmill 2.0?

Landed on hotfix-1.5: https://github.com/mozilla/mozmill/commit/ec09a8170c6c434dee1e5f5fc24b8559f346b34a

(In reply to Henrik Skupin (:whimboo) from comment #4) > (In reply to Henrik Skupin (:whimboo) from comment #2) > > This looks fine for Mozmill 1.5. I'm not sure if we can do such a thing for > > Mozmill 2.0 which also would need this fix. For that branch I would vote for > > moving the sleep() and waitFor() method from utils.js to mozmill.js. That > > way we can make use of the broker to send messages to the framework. > > Andrew, do you have any problems with that proposal for Mozmill 2.0? Any update on this? (I happily take this if :vladmaniac won't have the time for it.)

No one is working on it right now. So lets get the feedback from Andrew.

(In reply to Henrik Skupin (:whimboo) from comment #2) > This looks fine for Mozmill 1.5. I'm not sure if we can do such a thing for > Mozmill 2.0 which also would need this fix. For that branch I would vote for > moving the sleep() and waitFor() method from utils.js to mozmill.js. That > way we can make use of the broker to send messages to the framework. That sounds good to me

Could somebody point me to a test, or just provide a simplified testcase for this bug? Mentoring to create a testcase is also accepted! :) :whimboo, should I create a test.js for this in mutt? Or even enhance mutt/mutt/tests/js/utils/waitfor.js? Or even move waitfor.js to another path?

An early WIP stuff is available at GitHub. [1] I just moved the implementation to mozmill.js, and pointing it from utils.js. I didn't experience a major change in my testruns. So a testcase is really appreciated. [1]: https://github.com/xabolcs/mozmill/compare/branch-bug-795579-waitfor-passframe

(In reply to Szabolcs Hubai (:xabolcs) from comment #9) > Could somebody point me to a test, or just provide a simplified testcase for > this bug? A testcase is one of our Mozmill endurance tests which opens and closes windows. In that loop we never sent a frame via jsbridge. As result the connection timed out after the default global timeout. > :whimboo, should I create a test.js for this in mutt? > Or even enhance mutt/mutt/tests/js/utils/waitfor.js? Or even move waitfor.js > to another path? This should be a Python test becase you want to modify the global timeout, so the test runs only 1-2s instead of 60s. Thank you for taking care of it!

How can I run the endurance tests? I found testrun_endurance.py at https://hg.mozilla.org/qa/mozmill-automation, found the tests also at https://hg.mozilla.org/qa/mozmill-tests. mozmill master: 'thunderbird' : mozmill.ThunderbirdCLI, AttributeError: 'module' object has no attribute 'ThunderbirdCLI' After search & replacing mozmill.*CLI into mozmill.CLI, It starts checkouting but after that "prepare_tests" fails and I got: __init__() takes exactly 2 arguments (1 given) mozmill 1.5.19: parser_options = copy.copy(mozrunner.CLI.parser_options) AttributeError: type object 'CLI' has no attribute 'parser_options' My mozrunner is: $ mozrunner --version mozrunner-script.py 5.10

(In reply to Szabolcs Hubai (:xabolcs) from comment #12) > I found testrun_endurance.py at https://hg.mozilla.org/qa/mozmill-automation, > found the tests also at https://hg.mozilla.org/qa/mozmill-tests. The automation repository on hg is not compatible with Mozmill 2.0. You want our new repository from Github: https://github.com/whimboo/mozmill-automation. python setup.py develop testrun_endurance --repository=%path_to_tests% %path_to_application% > mozmill 1.5.19: > parser_options = copy.copy(mozrunner.CLI.parser_options) > AttributeError: type object 'CLI' has no attribute 'parser_options' > > My mozrunner is: > $ mozrunner --version > mozrunner-script.py 5.10 This is not a supported version for Mozmill 1.5. Create a new environment and setup the right mozrunner version as given in the setup.py of Mozmill.

Thanks! Seems like I have no luck with test_endurance.py :( Traceback (most recent call last): File "/path/to/mozmill-automation/mozmill_automation/testrun.py", line 265, in run self.run_tests() File "/path/to/mozmill-automation/mozmill_automation/testrun.py", line 471, in run_tests self._mozmill.add_listener(self.endurance_event, eventType='mozmill.enduranceResults') AttributeError: 'EnduranceTestRun' object has no attribute '_mozmill' With copy-pasteing mozmill init block from class TestRun(object).run_tests (~L186, 72b67c97f858 ) I was able to run endurance tests. The results are same with patch applied and without patch: RESULTS | Passed: 9 RESULTS | Failed: 2 RESULTS | Skipped: 3 No timeout problem, just "could not find element .... : ......"

This appear to have been broken since the update testrun was added here: https://github.com/whimboo/mozmill-automation/commit/f9655117f2a400dffc5bfb008887e724ee8de461 I have submitted a pull request that fixes this here: https://github.com/whimboo/mozmill-automation/pull/29 I then also discovered that the new window test was not included in the manifest.ini file so I reopened bug 711360 and attached a follow-up patch to fix this. Once all those issues were resolved, and I reduced the manifest file locally to just the open new window test, I was able to replicate this issue with Mozmill 2.0 using the following command line: $ testrun_endurance --repository=~/workspace/mozmill-tests/default --report=http://mozmill-crowd.blargon7.com/db/ --delay=180 /Applications/FirefoxNightly.app/ *** Application: Firefox 20.0a1 *** Platform: Mac OS X 10.8.2 64bit Preparing mozmill-tests repository... requesting all changes adding changesets adding manifests adding file changes added 2767 changesets with 7125 changes to 757 files (+5 heads) updating to branch default 403 files updated, 0 files merged, 0 files removed, 0 files unresolved pulling from ~/workspace/mozmill-tests/default searching for changes no changes found 0 files updated, 0 files merged, 0 files removed, 0 files unresolved 2012-11-24 15:12:18.644 firefox[20260:707] invalid drawable TEST-START | testTabbedBrowsing_OpenNewWindow/test1.js | setupModule TEST-START | testTabbedBrowsing_OpenNewWindow/test1.js | testOpenAndCloseMultipleWindows 2012-11-24 15:15:21.638 firefox[20260:707] invalid drawable Timeout: bridge.execFunction("58087d80-3649-11e2-8321-3c0754427713", bridge.registry["{2ecfbce9-eaf2-a04e-89e3-ca887af9cf88}"]["runTestFile"], ["/var/folders/5_/9gz0dv9s51q2shxc8hg62j200000gn/T/tmpdB_gI8.mozmill-tests/tests/endurance/testTabbedBrowsing_OpenNewWindow/test1.js", null]) Timeout: bridge.set("53db470a-364a-11e2-bd18-3c0754427713", Components.utils.import("resource://mozmill/driver/mozmill.js")) Traceback (most recent call last): File "/Users/dhunt/workspace/mozmill-automation-gh/mozmill_automation/testrun.py", line 268, in run self.run_tests() File "/Users/dhunt/workspace/mozmill-automation-gh/mozmill_automation/testrun.py", line 365, in run_tests TestRun.run_tests(self) File "/Users/dhunt/workspace/mozmill-automation-gh/mozmill_automation/testrun.py", line 220, in run_tests self._mozmill.run(tests, self.options.restart) File "/Users/dhunt/workspace/mozmill/master/mozmill/mozmill/__init__.py", line 410, in run self.stop_runner() File "/Users/dhunt/workspace/mozmill/master/mozmill/mozmill/__init__.py", line 499, in stop_runner raise Exception('client process shutdown unsuccessful') Exception: client process shutdown unsuccessful

(In reply to Dave Hunt (:davehunt) from comment #15) > > ... > > Once all those issues were resolved, and I reduced the manifest file locally > to just the open new window test, I was able to replicate this issue with > Mozmill 2.0 using the following command line: > > $ testrun_endurance --repository=~/workspace/mozmill-tests/default > --report=http://mozmill-crowd.blargon7.com/db/ --delay=180 > /Applications/FirefoxNightly.app/ > ... > Thank you very much for your comment! Would you mind to make a testrun with my development branch (in #10)? In the mean tim I will try to set up a patched environment based on your description. Could you also pm me, how to reduce the manifest file, and how to "run" that (should i commit the changes, ...). Because when I left only one line in it, then the suite didn't run any tests, and finished with 0/0/0 result.

(In reply to Szabolcs Hubai (:xabolcs) from comment #16) > Thank you very much for your comment! > Would you mind to make a testrun with my development branch (in #10)? > In the mean tim I will try to set up a patched environment based on your > description. Sure, I will try to do this on Monday. > Could you also pm me, how to reduce the manifest file, and how to "run" that > (should i commit the changes, ...). > Because when I left only one line in it, then the suite didn't run any tests, > and finished with 0/0/0 result. You probably got it right but check the name of the test you reduced it to. The manifest has an error in it and lists the open window test under TabView instead of TabbedBrowsing. That said, any test with a high delay should cause this issue. You just need the reduced manifest in an applied patch, and refer to the local repository when running the tests.

(In reply to Dave Hunt (:davehunt) from comment #15) > ... > > Once all those issues were resolved, and I reduced the manifest file locally > to just the open new window test, I was able to replicate this issue with > Mozmill 2.0 using the following command line: > > $ testrun_endurance --repository=~/workspace/mozmill-tests/default > --report=http://mozmill-crowd.blargon7.com/db/ --delay=180 > /Applications/FirefoxNightly.app/ > ... Phew! I was able to replicate this, too. Thank you again for the detailed steps! Testing with the development branch (see comment #10): > TEST-START | testTabbedBrowsing_OpenNewWindow/test1.js | setupModule > TEST-START | testTabbedBrowsing_OpenNewWindow/test1.js | testOpenAndCloseMultipleWindows > TEST-PASS | testTabbedBrowsing_OpenNewWindow/test1.js | testOpenAndCloseMultipleWindows > TEST-START | testTabbedBrowsing_OpenNewWindow/test1.js | teardownModule > TEST-END | testTabbedBrowsing_OpenNewWindow/test1.js | finished in 540424ms > RESULTS | Passed: 1 > RESULTS | Failed: 0 > RESULTS | Skipped: 0 > So the proof-of-concept is working. :) My next question is how to finish this fix? I mean: - it's this fix-by-redirect (commit 51e5be647ce8 [1]) enough - or should I point all the utils.js:sleep / utils.js:waitFor usage to mozmill.js too? - - and should I extend utils.js:.sleep, waitFor to drop a "depreacated" line to avoid future use? (In reply to Henrik Skupin (:whimboo) from comment #11) > (In reply to Szabolcs Hubai (:xabolcs) from comment #9) > > :whimboo, should I create a test.js for this in mutt? > > Or even enhance mutt/mutt/tests/js/utils/waitfor.js? Or even move waitfor.js > > to another path? > > This should be a Python test becase you want to modify the global timeout, > so the test runs only 1-2s instead of 60s. > > Thank you for taking care of it! I'm totally newbie in writing python tests. I really can't imagine how should be this tested. :( Henrik, could You help me? Where should I start reading the mutt/mutt/tests/python directory for reference? [1]: compare view - https://github.com/xabolcs/mozmill/compare/51e5be647ce8

(In reply to Szabolcs Hubai (:xabolcs) from comment #18) > > [...] > > So the proof-of-concept is working. :) > My next question is how to finish this fix? > I mean: > - it's this fix-by-redirect (commit 51e5be647ce8 [1]) enough > - or should I point all the utils.js:sleep / utils.js:waitFor usage to > mozmill.js too? > - - and should I extend utils.js:.sleep, waitFor to drop a "depreacated" > line to avoid future use? > > > (In reply to Henrik Skupin (:whimboo) from comment #11) > > (In reply to Szabolcs Hubai (:xabolcs) from comment #9) > > > :whimboo, should I create a test.js for this in mutt? > > > Or even enhance mutt/mutt/tests/js/utils/waitfor.js? > > > Or even move waitfor.js to another path? > > > > This should be a Python test becase you want to modify the global timeout, > > so the test runs only 1-2s instead of 60s. > > > > Thank you for taking care of it! > > I'm totally newbie in writing python tests. > I really can't imagine how should be this tested. :( > Henrik, could You help me? > > Where should I start reading the mutt/mutt/tests/python directory for > reference? > > > [1]: compare view - https://github.com/xabolcs/mozmill/compare/51e5be647ce8 Henrik, would you mind to answer or redirect my question? Thanks!

(In reply to Szabolcs Hubai (:xabolcs) from comment #19) > > Henrik, would you mind to answer or redirect my question? > Thanks! Ping.

(In reply to Szabolcs Hubai (:xabolcs) from comment #19) Sorry for the late reply on it! > > I'm totally newbie in writing python tests. > > I really can't imagine how should be this tested. :( > > Henrik, could You help me? > > > > Where should I start reading the mutt/mutt/tests/python directory for > > reference? Mutt is using the manifest files in general. So there is not that much of a difference to the JS tests you have already run. Just use the following command line: mutt testpy -m manifest_file Whereby manifest_file will be that one: https://github.com/mozilla/mozmill/blob/master/mutt/mutt/tests/python/tests.ini When you want to test an individual test simply comment out all the others. For the tests themselves we make use of the Python unittest framework: http://docs.python.org/2/library/unittest.html I hope that helps you to get started with the test. And sorry again for the delayed reply.

Testing locally, without broker.pass({...}) my python test fails: ERROR: test_waitfor_send_pass_frame (test_bug795579.TestBug795579) ---------------------------------------------------------------------- Traceback (most recent call last): File "/path/to/mozmill/mutt/mutt/tests/python/test_bug795579.py", line 19, in test_waitfor_send_pass_frame self.do_test(testpath, passes=1, fails=0, skips=0) File "/path/to/mozmill/mutt/mutt/tests/python/test_bug795579.py", line 27, in do_test m.run(tests) File "/path/to/mozmill/mozmill/mozmill/__init__.py", line 423, in run self.stop_runner() File "/path/to/mozmill/mozmill/mozmill/__init__.py", line 518, in stop_runner raise Exception('client process shutdown unsuccessful') Exception: client process shutdown unsuccessful With broker.pass() the test passes. :) (Will attach the patch soon) I still have questions: - is this fix-by-redirect (commit 51e5be647ce8 [1]) enough - or should I point all the utils.js:sleep / utils.js:waitFor usage to mozmill.js too? - - and should I extend utils.js:.sleep, waitFor to drop a "depreacated" line to avoid future use? [1]: compare view - https://github.com/xabolcs/mozmill/compare/51e5be647ce8

Tests. There are two tests commented out. They use too long timeouts for JSBRIDGE. Those test will be passing only if waitFor and sleep sends "keepalive"-like frames too, not just "pass" frames. Are there any "keepalive"-like frames in Mozmill? Or should I discard those tests? They are already commented out. Compare View - https://github.com/xabolcs/mozmill/compare/f0ec336 (BTW could somebody let me know how to export this from mercurial?)

Szabolcs is working on this bug. Not sure why the assignee got changed here. Reverting back. This is not a sprint for SV.

In bug 791634 we're adding waitFor to the assertions module, and in this bug we're moving them to mozmill.js. Should one bug block the other to avoid any duplication of effort? (In reply to Szabolcs Hubai (:xabolcs) from comment #23) > Compare View - https://github.com/xabolcs/mozmill/compare/f0ec336 > (BTW could somebody let me know how to export this from mercurial?) What are you trying to do here? Mozmill is in a git repository so you shouldn't need Mercurial.

Comment on attachment 737784 [details] [diff] [review] compare-master-to-f0ec336.diff Review of attachment 737784 [details] [diff] [review]: ----------------------------------------------------------------- ::: mozmill/mozmill/extension/resource/driver/mozmill.js @@ +291,5 @@ > + * > + * @param {number} milliseconds > + * Sleeps the given number of milliseconds > + */ > +function sleep(milliseconds) { Why are you moving those methods out of utils.js? They should stay where they were. Or is there any reason for? ::: mutt/mutt/tests/python/js-tests/test_bug795579.js @@ +12,5 @@ > + for (var i = 0; i < gSecondsToWait; i++){ > + controller.sleep(1000); > + } > + expect.pass("testSleep passes"); > +} Do we really need a test for sleep? Does the test fail if we are waiting 8s at once? @@ +21,5 @@ > + expect.doesNotThrow(function () { > + controller.waitFor(function () { > + return Date.now() > timeToBailOut; > + }); > + }, "TimeoutError", "waitFor() has to pass."); We should set a higher value (best twice as the global timeout) as timeout for the waitFor call, and repeat the loop each 100ms. Once we are close to the waitFor timeout, the callback can return true. So we can make that construct way simpler. @@ +38,5 @@ > + controller.waitFor(function () { > + return Date.now() > timeToBailOut; > + }, undefined, (gSecondsToWait + 1) * 1000); > + }, "TimeoutError", "waitFor() has to pass when the time is come."); > + expect.pass("testBigWaitFor passes"); This can be removed? ::: mutt/mutt/tests/python/test_bug795579.py @@ +10,5 @@ > + """Bug 795579: waitFor method doesn't send a pass frame > + which can cause an application disconnect when handled in a loop > + """ > + > + jsbridge_timeout = 6. I think we might want to increase it a bit e.g. 15s? Otherwise tests might fail if it takes longer to launch Firefox during tests, e.g. for debug builds. ::: mutt/mutt/tests/python/tests.ini @@ +5,4 @@ > > [expectstacktest.py] > [test_bug690154.py] > +[test_bug795579.py] Would you mind to use a real description as part of the filename? That makes it easier to see what a test is about.

Addressed comments: - drop long running tests - increase jsbridge_timeout to 15s - borrow logDeprecated from controller.js to log deprecation warnings - renamed *.py The above changes are currently untested. The references aren't updated so requesting feedback only. Compare View - https://github.com/xabolcs/mozmill/compare/f2163a41dd28

Comment on attachment 738846 [details] [diff] [review] compare-master-to-f2163a41dd28.diff Cancelling feedback. I noted to Henrik that the implemented _logDeprecated() function imports msgbroker.js locally, and works correctly in utils.js. So I wondered that the implementation wouldn't be needed to move to mozmill.js. That's why I cancelled the request. I'm going to revert my first commit on the branch, and update the patch. After that, the fix for 2.0 going to be very similar to 1.5.

As noted in my previous comment this patch contains - new (private) broker object in utils.js - the waitFor() and sleep() now send the pass frame with the help of the new broker - the test from previous patch

Comment on attachment 740439 [details] [diff] [review] compare-master-to-b3d7188bfd39.diff Review of attachment 740439 [details] [diff] [review]: ----------------------------------------------------------------- That looks way better, yes. Thanks for all the information you gave back on this issue. I should have checked the details first before my previous review on this bug. There are only some nits we should get addressed before it can be landed. ::: mozmill/mozmill/extension/resource/stdlib/utils.js @@ +199,5 @@ > while (!timeup) { > thread.processNextEvent(true); > } > + > + broker.pass({'function':'mozmill.sleep()'}); nit: 'utils.sleep' @@ +296,4 @@ > throw new TimeoutError(message); > } > > + broker.pass({'function':'mozmill.waitFor()'}); nit: utils.waitFor ::: mutt/mutt/tests/python/js-tests/test_bug795579.js @@ +29,5 @@ > + expect.doesNotThrow(function () { > + controller.waitFor(function () { > + return Date.now() > timeToBailOut; > + }, undefined, gSecondsToWait * 1000 * 2, 100); > + }, "TimeoutError", "waitFor() has to pass."); We don't want to test for a TimeoutError here, but that jsbridge is constantly getting the 'virtual' heartbeat. Therefore it would be enough to have a waitFor with a timeout and delay of 1000. So we return always after about 1s. That way we wouldn't have to do the check with the timeToBailOut and don't need an assertions. ::: mutt/mutt/tests/python/test_bug795579_waitFor_pass_frame.py @@ +29,5 @@ > + m = mozmill.MozMill.create(jsbridge_timeout=self.jsbridge_timeout) > + m.run(tests) > + results = m.finish() > + > + # From the first test, there is one passing test Hm, the comment mismatches with the parameter as set for this method. There 2 tests should pass. What's up with that? ::: mutt/mutt/tests/python/tests.ini @@ +5,4 @@ > > [expectstacktest.py] > [test_bug690154.py] > +[test_bug795579_waitFor_pass_frame.py] I would kill the bug number from the filename. It's enough to have it as comment in the test.

Addressed nits - mozmill.* copy-paste - slimmer waitFor test - removed needless comment - renamed *.py

Comment on attachment 741566 [details] [diff] [review] compare-master-to-be915064e8d8.diff Review of attachment 741566 [details] [diff] [review]: ----------------------------------------------------------------- Everything looks fine now. But you missed to rename that JS test file. Can you please do it so we can get this patch landed? Thanks.

Renamed js, but not tested, forwarding whimboo's r+.

(In reply to Szabolcs Hubai (:xabolcs) from comment #33) > Created attachment 742259 [details] [diff] [review] > compare-master-to-fc5561f7ecee.diff > > Renamed js, but not tested, forwarding whimboo's r+. Now it's tested.

Comment on attachment 742259 [details] [diff] [review] compare-master-to-fc5561f7ecee.diff Review of attachment 742259 [details] [diff] [review]: ----------------------------------------------------------------- This patch has carriage returns all over the place. Also the format is not as requested by our docs. Please export the patch with git show like given here: https://wiki.mozilla.org/Auto-tools/Projects/Mozmill/RepoSetup#Ready_for_a_review

Also could we shorten the time to run the test? Right now it's strange to see Firefox open for 36s without doing anything. I think we can safely reduce the max wait time to 5s or so. Also loading a test page might be helpful to explain what's taken so long for the test.

(In reply to Henrik Skupin (:whimboo) from comment #35) > > This patch has carriage returns all over the place. Also the format is not > as requested by our docs. Please export the patch with git show like given > here: > https://wiki.mozilla.org/Auto-tools/Projects/Mozmill/ > RepoSetup#Ready_for_a_review Thanks for the info! (In reply to Henrik Skupin (:whimboo) from comment #36) > Also could we shorten the time to run the test? Right now it's strange to > see Firefox open for 36s without doing anything. I think we can safely > reduce the max wait time to 5s or so. Also loading a test page might be > helpful to explain what's taken so long for the test. You asked me to longer jsbridge_timeout to 15s in comment #26. Yes it's really a long test, I'll test with some debug builds, and try to shorten the timeout. But please note, that these tests should take longer than the jsbridge_timeout waits. I have two tests in this patch, so it will run about 2x jsbridge_timeout. If you reduce the running time of the tests below the jsbridge_timeout, they will test nothing. (And if you use longer waits than jsbridge_timeout in the loop, then it will disconnect - which is a known fail, as talked on the irc.) A testpage will be very helpful. Thanks! :)

We always have the possibility to reduce that jsbridge timeout. As you say we could run into side-effects. So having a test page would be the preferred way.

It's based on the previous patch with countdown feature. :) Henrik, please provide a feedback, it's this simple "gui" enough? Or should I drop a first line "waitFor tests are in progress..."? Thanks. Yeah, I know, I used obsoleted API, but I'm happy because it's working. :) There was time when it wasn't. I'll update it soon. One more thing, the setupModule. If I don't wait for pageload in the setupModule, then the first test will fail, and only the second pass. After I waits for Firefox to load it's startup page, the simplediv.html loads will fine then, and both test will pass. Any thoughts?

Comment on attachment 744250 [details] [diff] [review] compare-master-to-35f6975f79ce.patch Review of attachment 744250 [details] [diff] [review]: ----------------------------------------------------------------- Please provide a full patch not an interdiff. Thanks.

IMHO the obsoleted attachment was a full patch. Indeed here is the mq managed patch. It should cleanly apply to https://github.com/mozilla/mozmill/commit/445780f2acfc ! It's now uses findElement.ID!

Comment on attachment 744842 [details] [diff] [review] rebased, collapsed patch against master v2 Review of attachment 744842 [details] [diff] [review]: ----------------------------------------------------------------- ::: mozmill/mozmill/extension/resource/stdlib/utils.js @@ +295,5 @@ > message = message || arguments.callee.name + ": Timeout exceeded for '" + callback + "'"; > throw new TimeoutError(message); > } > > + broker.pass({'function':'utils.waitFor()'}); When I remove those lines the current test as is doesn't fail. So we don't correctly test it. ::: mutt/mutt/tests/python/js-tests/test_waitFor_pass_frame.js @@ +12,5 @@ > + node.innerHTML = aMsg; > + aDiv.parentNode.appendChild(node); > + }; > + // why is this needed? > + controller.waitForPageLoad(); This is not necessary. It will make the test fail completely because we are stuck in that method. Hm, also while checking this patch I can see that we add a lot of code for logging. I thought we could easily do it by using controller.tabs.activeTab.write(), but it seems like it's not allowed anymore across compartments (especially between chrome and content). Would it be ok for you if we skip the logging for now? With the updated time it shouldn't be that long anymore. @@ +16,5 @@ > + controller.waitForPageLoad(); > +} > + > +// this timeout should be more than |jsbridge_timeout| > +var gSecondsToWait = 4; I think the difference between the jsbridge timeout and this one is too small. Shouldn't we better leave it at 3s as we had before?

Contents: - testing utils.* instead controller.* - removed unused and failing setupModule - increased timeout

Comment on attachment 748566 [details] [diff] [review] rebased, collapsed patch against master v3 Review of attachment 748566 [details] [diff] [review]: ----------------------------------------------------------------- Thank you for the updated patch. There is one thing I would like to know before a final signoff. I can remember that you mentioned issues when running waitFor tests after the ones for sleep. Were you able to figure out what was broken? With that question answered we can land it.

(In reply to Henrik Skupin (:whimboo) from comment #44) > > Thank you for the updated patch. There is one thing I would like to know > before a final signoff. I can remember that you mentioned issues when > running waitFor tests after the ones for sleep. Were you able to figure out > what was broken? With that question answered we can land it. Yes, that problem was what you pointed out in comment #42, see below: > ::: mozmill/mozmill/extension/resource/stdlib/utils.js > @@ +295,5 @@ > > message = message || arguments.callee.name + ": Timeout exceeded for '" + callback + "'"; > > throw new TimeoutError(message); > > } > > > > + broker.pass({'function':'utils.waitFor()'}); > > When I remove those lines the current test as is doesn't fail. So we don't > correctly test it. > The problem was in prior patches that I tested controller.sleep() and controller.waitFor(), instead testing utils.* directly. The difference was caused by controller.sleep() is === utils.sleep() [1], while controller.waitFor() equals to (utils.sleep + broker.pass("controller.waitFor()")). [2] That caused the waitFor test pass even without the fix. Should any follow-up work be done with controller.sleep() and controller.waitFor()? [1]: https://github.com/mozilla/mozmill/blob/fd4739f85fd7/mozmill/mozmill/extension/resource/driver/controller.js#L273 [2]: https://github.com/mozilla/mozmill/blob/fd4739f85fd7/mozmill/mozmill/extension/resource/driver/controller.js#L363

whimboo: ping. Was my answer acceptable? (In reply to Szabolcs Hubai (:xabolcs) from comment #45) > (In reply to Henrik Skupin (:whimboo) from comment #44) > > <snip> > > Should any follow-up work be done with controller.sleep() and > controller.waitFor()? Any thoughts about this?

Where are we on this bug, Henrik? Looks like it's ready to land?

(In reply to Szabolcs Hubai (:xabolcs) from comment #45) > The problem was in prior patches that I tested controller.sleep() > and controller.waitFor(), instead testing utils.* directly. > > The difference was caused by controller.sleep() is === utils.sleep() [1], > while controller.waitFor() equals to (utils.sleep + > broker.pass("controller.waitFor()")). [2] > That caused the waitFor test pass even without the fix. Oh, interesting. So please kill this extra broker.pass line in controller. We would get two pass frames with your change now, which is not that good. With that fix we are good to go. I don't see a reason for a follow-up bug.

Now controller.waitFor() === utils.waitFor()

Comment on attachment 757253 [details] [diff] [review] patch v4 Review of attachment 757253 [details] [diff] [review]: ----------------------------------------------------------------- Yes, that looks better now. It's ready to get landed. Thanks for the update.

Landed on master: https://github.com/mozilla/mozmill/commit/bd030d15776f506c2a97db6068fd6ccaa96a63a9

(In reply to Henrik Skupin (:whimboo) from comment #51) > Landed on master: > https://github.com/mozilla/mozmill/commit/bd030d15776f Something wrong happened here: where is the tests? Did I forget from my patch? Ouch!

Damn. One of the reasons why I really like pull requests instead of attachments. Sorry, about that and good that you were checking the landing! Pushed the tests to master: https://github.com/mozilla/mozmill/commit/4677a09711ffccfc1aee2c1d8558d609a18b6935