761605 - mutt/tests/js/testFrame/testGlobalTimeout has to be a python test

Status: RESOLVED WONTFIX

(Testing Graveyard :: Mozmill, defect)

Description

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

Right now this tests lives under the js tests and should probably moved out to the Python tests. I don't want to wait 60s for the default global timeout when running the js tests. Via Python we can improve the test by changing the time to 10s or so.

It depends on my work on bug 760418.

Comment 1

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

So this cannot be a Python (Mutt) test because mutt always restarts the application in between tests. In the above case this should not happen.

Clint, is there a way to prevent mutt from doing so, like specifying a 'no-restart' option in the manifest file?

Comment 2

[Anand Soni]

Henrik,

What do we need to do? Should I write all the tests in python? Need a bit more clarification. Thank you for suggesting this!

Comment 3

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

So the js tests you can find under mutt/mutt/tests/js/frame/global_timeout/ have to be moved to python/js-modules. Then an additional py test has to be created inside of python/. This test will make use of the formerly moved js tests, and adds appropriate checks for the expected amount of passed and failed tests.

Comment 4

[Anand Soni]

I didn't completely understand. How am I to create the new Python test? What is its purpose? Which are the tests to be checked for pass/fail?

Comment 5

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

So the global timeout setting does exist because the Python part of Mozmill has to be able to kill the application under test if it doesn't respond in a given amount of time. The timeout can be set via the command line (check the options with --help) or via the Mozmill API (Python). What we want here is a Python test which sets the timeout to a shorter value and checks that the application gets killed once we run into the timeout. A test you can use as template and which also makes use of the template is test_waitfor_pass_frame.py. Please have a look in how it works. The test to be written here has to look similar in terms of py and js test file locations, and running the test. Only the final checks will be different.

Please get familiar with that and once done let us talk about which timeout situations we want to test, and which final checks we might want to do.

Comment 6

[Anand Soni]

Sorry for the delay. I was away for a while. I will look into it now! :)

Comment 7

[Sambuddha Basu [:sambuddhabasu1]]

Henrik, I would like to work on this bug. It would be great if you can assign it to me.

Comment 8

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

Absolutely! And thank you for your interest. With the move of the test our suite will become more confident given that we then can test the right bits.

Comment 9

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

Hi Sambuddha. Would you mind to give us an update for this bug? Are you still interested to work on? If not we want to re-assign once someone with interest requests assignment. Thanks.

Comment 10

[Sambuddha Basu [:sambuddhabasu1]]

Hi Henrik. As I will be having a busy schedule till the end of this month, I wont be able to get my hands on this bug till then. After that, I will be able to get back on this. Till then, if anyone else wants to work on this, feel free to assign it to them. Thanks.

Comment 11

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

Sam, I assume you also want to continue on this bug?

Comment 12

[Sambuddha Basu [:sambuddhabasu1]]

Yes, I want to continue with this bug.

Comment 13

[Sambuddha Basu [:sambuddhabasu1]]

Attachment: 0001-Bug-761605-mutt-tests-js-testFrame-testGlobalTimeout.patch

Comment 14

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

Comment on attachment 8482417 [details] [diff] [review]
0001-Bug-761605-mutt-tests-js-testFrame-testGlobalTimeout.patch

Review of attachment 8482417 [details] [diff] [review]:
-----------------------------------------------------------------

So far it is a good start. Files have been correctly moved, and the Python test created. Nevertheless the the Python test needs an addition to be fully testing this feature. See my inline comment.

::: mutt/mutt/tests/python/test_global_timeout.py
@@ +15,5 @@
> +    jsbridge_timeout = 10.
> +
> +    def test_global_timeout(self):
> +        testpath = os.path.join("js-modules", "testGlobalTimeout", "manifest.ini")
> +        self.do_test(testpath, passes=0, fails=1, skips=0)

What you also wanna do here is to check how long the test actually run. Given that we are modifying the jsbridge timeout, the browser should have closed within that period of inactivity. Means for the first test you want to set a time on the persisted object, and evaluate it here. It's similar to the test I have created here:

https://github.com/mozilla/mozmill/blob/master/mutt/mutt/tests/python/test_shutdown_delayed.py

Comment 15

[Sambuddha Basu [:sambuddhabasu1]]

Attachment: 0001-Bug-761605-mutt-tests-js-testFrame-testGlobalTimeout.patch

Comment 16

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

Comment on attachment 8484993 [details] [diff] [review]
0001-Bug-761605-mutt-tests-js-testFrame-testGlobalTimeout.patch

Review of attachment 8484993 [details] [diff] [review]:
-----------------------------------------------------------------

::: mutt/mutt/tests/python/js-modules/testGlobalTimeout/testTimeout.js
@@ +11,5 @@
> + * Global timeout should allow to proceed the next test instead of killing
> + * the complete test-run
> + */
> +function test() {
> +  controller.sleep(persisted.waitTime);

The usage of persisted.waitTime is not necessary here. It doesn't matter how long we sleep, as long as it it longer than the jsbridge timeout. That means if we use 10s, the sleep should take at least 20s for safety. Best would be to have a sleep of 60s here. and get rid of the persisted object.

::: mutt/mutt/tests/python/test_global_timeout.py
@@ +15,5 @@
> +
> +class TestGlobalTimeout(unittest.TestCase):
> +
> +    def test_global_timeout(self):
> +        persisted = {'waitTime': TIMEOUT * 1000}

As said on the other file, this is not necessary. It would even cause a test failure, because the jsbridge timeout was also 10s, and for the browser it takes a bit to close.

@@ +45,5 @@
> +        m.run(tests)
> +        results = m.finish()
> +
> +        self.assertEqual(len(results.passes), passes)
> +        self.assertEqual(len(results.fails), fails)

You misread my last review. My idea was to simply point you to an example how to calculate the time used for the test, or in this case when mozmill kills the browser due to a jsbridge timeout. In this version of the patch you largely replaced the whole content with what you found in the other test. Please re-check and compare the last version of the patch against the given example. Then add the parts which calculates the time between the start of the test and time_end.

Comment 17

[Sambuddha Basu [:sambuddhabasu1]]

Attachment: 0001-Bug-761605-mutt-tests-js-testFrame-testGlobalTimeout.patch

Comment 18

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

Comment on attachment 8488557 [details] [diff] [review]
0001-Bug-761605-mutt-tests-js-testFrame-testGlobalTimeout.patch

Review of attachment 8488557 [details] [diff] [review]:
-----------------------------------------------------------------

::: mutt/mutt/tests/python/js-modules/testGlobalTimeout/testTimeout.js
@@ +9,5 @@
> +/**
> + * Bug 584470
> + * Global timeout should allow to proceed the next test instead of killing
> + * the complete test-run
> + */

Lets get this comment removed. Another set of tests will be added for this particular bug.

@@ +11,5 @@
> + * Global timeout should allow to proceed the next test instead of killing
> + * the complete test-run
> + */
> +function test() {
> +  controller.sleep(3600000);

nit: the time format as used by sleep is already seconds, so please reduce it to 3600. The current value won't hurt, but its hard for readability and not necessary.

::: mutt/mutt/tests/python/js-modules/testGlobalTimeout/testTimeoutAfter.js
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, you can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +function test() {
> +}

This test only exists because of the comment on the other js test file. It's not necessarily needed for this test. So I would suggest that we get rid of it.

::: mutt/mutt/tests/python/test_global_timeout.py
@@ +18,5 @@
> +        testpath = os.path.join("js-modules", "testGlobalTimeout", "testTimeout.js")
> +        results = self.do_test(testpath, exception=mozmill.errors.ShutdownError,
> +                               passes=0, fails=1, skips=0)
> +
> +        self.assertTrue((results.endtime - results.starttime).seconds < 3600)

This is not the correct check here. Please see the global jsbridge timeout above. It's set to 10s and not 3600s. Also the default value is 60s, so the duration we get here should be a bit longer than 10s but way shorter than 60s.

Comment 19

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

Sam asked me on IRC how to compare those times, and we checked some bits. Finally I found out why the time Firefox is open is still nearly a minute. The problem is that Mozmill does not directly shutdown Firefox, but it takes a while. So for this test to properly work we need bug 794020 fixed first.

Comment 20

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

Mozmill will reach its end of life soon. We are currently working on getting
all the tests for Firefox ported to Marionette. For status updates please see
bug 1080766.