Zac C (:zac) Assignee
	Description • 10 years ago

Follow up to https://bugzilla.mozilla.org/show_bug.cgi?id=1058158

In that bug CommonTestCase was modified to switch into test_container app for Marionette WebAPI tests. 

Some of the change for that has affected the Gaia UI tests.


(In reply to Malini Das [:mdas] from comment #20)
> Created attachment 8487489 [details] [diff] [review]
> run in test-container, remove oop option
> 
> the only change from the previous patch is line 388 of marionette_test.py.
> This patch ran green on Mn and Mnw

This change is also trying to run in Gaia UI tests and causing some issues.

Can we modify setUp to only execute close_test_container when the test_container attribute is present? We want to avoid executing script whereever we can, especially javascript code such as that which really should only be in the Gaia repo to avoid it going stale.

http://dxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette/marionette_test.py#390

	Zac C (:zac) Assignee
	Comment 1 • 10 years ago

Mdas or jgriffin, can you guys work on this as a follow-up to the bug that introduced this issue?

	Malini Das [:mdas] - Away, not checking bugmail
	Comment 2 • 10 years ago

(In reply to Zac C (:zac) from comment #0)

> Can we modify setUp to only execute close_test_container when the
> test_container attribute is present? We want to avoid executing script
> whereever we can, especially javascript code such as that which really
> should only be in the Gaia repo to avoid it going stale.

Since we want to keep the test container app running between tests to reduce test time, we keep the test container open until we see a test that doesn't need it, then we close it. The setUp approach won't work because if the last Mn test has test_container = true and then your first gaiatest has that set to false, then we'd be executing the close_test_container app during your suite.

To keep suites separated, I can instead modify our test runner to close the app when it knows that the next test that will run doesn't need the test container. By keeping track of the previous run test's test_container value, when we're about to run the next test here https://mxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette/runner/base.py#783, we can check if the test to be executed needs the test_container. If it doesn't, then the runner can kill it.

jgriffin, what do you think?

	Jonathan Griffin (:jgriffin)
	Comment 3 • 10 years ago

I think that would be ok, unless we can just leave the test-container open and not kill it at all.

	Malini Das [:mdas] - Away, not checking bugmail
	Comment 4 • 10 years ago

(In reply to Jonathan Griffin (:jgriffin) from comment #3)
> I think that would be ok, unless we can just leave the test-container open
> and not kill it at all.

Thanks, I'd rather avoid that, and make sure subsequent suites are in a known/predictable state. I can fix this today

	Malini Das [:mdas] - Away, not checking bugmail
	Comment 5 • 10 years ago

will mark for r? when try is green.

try: https://tbpl.mozilla.org/?tree=Try&rev=6df08297324a

	Malini Das [:mdas] - Away, not checking bugmail
	Comment 6 • 10 years ago

there's an Mn failure on emulator I can't reproduce locally. I have to rebuild my emulator. This is going to be fun.

	Dave Hunt [:davehunt] [he/him] ⌚BST
	Comment 7 • 10 years ago

While profiling the test runner for bug 1038868 I noticed that this close_test_container is adding ~10 second for each test. We restart between tests so if there's an initial cost it will be incurred every time.

	Zac C (:zac) Assignee
	Comment 8 • 10 years ago

Here's a more lightweight variation that is less likely to impact existing test suites. 
I'll have to follow this up with setting test_container to None inside gaiatest.

Try:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=640597193a93

	Dave Hunt [:davehunt] [he/him] ⌚BST
	Comment 9 • 10 years ago

Comment on attachment 8505443 [details] [diff] [review]
bug_1075451.diff

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

I'm not keen on adding any command line options for this. I may have misunderstood, but it would appear that a test indicates that it uses the test container in the manifest file. This would mean that none of the gaia UI tests will try to switch into the test container because they do not indicate the requirement. This suggests that the issue here is in closing the test container. I haven't closely read through the code, but perhaps there's an issue in the close_test_container method that's causing it to timeout? Any such timeout exceptions in setUp would be caught by gaiatest due to the crash detection code.

As this code is specific to a suite of tests, could it make sense to extend the runner instead of having it in CommonTestCase?

	Zac C (:zac) Assignee
	Comment 10 • 10 years ago

The code has True/False flows and both execute action against the test container. There's no option for 'None' to skip it altogether which I tried to add here.

When we run with the restart flag it attempts to execute the code before gaiatest has even started up B2G so I can't really put a condition in to check something in Gaia.

If you're not running with restart then I'm sure the code is fine because Gaia is always running.

I agree this should be inside the test suite's code. I'll have a look from that angle later today.

	Zac C (:zac) Assignee
	Comment 11 • 10 years ago

Comment on attachment 8498385 [details] [diff] [review]
test_con

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

::: testing/marionette/client/marionette/runner/base.py
@@ +669,5 @@
> +        self.marionette.switch_to_frame(frame)
> +        self.marionette.delete_session()
> +
> +    """
> +    Used by b2g device tests. Closes the test container app if it is running

I think in this comment you mean to say "emulator tests" ? 
The device tests definitely don't use this container.

	Zac C (:zac) Assignee
	Comment 12 • 10 years ago

(In reply to Malini Das [:mdas] from comment #6)
> there's an Mn failure on emulator I can't reproduce locally. I have to
> rebuild my emulator. This is going to be fun.

I've managed to replicate this locally.
What I've found is that after the test does self.marionette.navigate , marionette can no longer reach navigator.mozSettings API and hence close_test_container fails where it checks that navigator.mozSettings is not null.

Thus the test before test_click is leaving the device in a bad state. It is probably a Gaia security feature that Mn cannot reach mozSettings if the url is not that of an app with permissions to do so.

I suspect with the old code that switch_into_test_container did not ever work as if I replace that with a 'pass' then the tests carry on passing (mostly).

I tried a variant of switch_into_test_container without mozSettings which worked but then it found that navigator.mozApps, while present, does not seem to grant access to navigator.mozApps.mgmt.

self.marionette.navigate seems to be too aggressive in unloading the test container and Gaia, although the tests are currently passing in spite of that.

So at this point I need some guidance as to the next step to take, mdas and jgriffin, please, I'm happy to keep working with guidance as to what Mn tests need.

	Malini Das [:mdas] - Away, not checking bugmail
	Comment 13 • 10 years ago

As Zac mentioned on IRC, there's a problem with setUp/tearDown since they stop and start marionette sessions without switching into test-container app when a new session starts. I think the solution here is to pass the test_container variable to the MarionetteTestCase in marionette_test.py like we currently do, and instead of calling the current switch_into_test_container function (which will launch the test container if it exists), we will instead have that function switch into the app. This way, base.py is in charge of the actual launching and closing of the test container, and the MarionetteTestCase is responsible for switching into the test container app if needed.

	Jonathan Griffin (:jgriffin)
	Comment 14 • 10 years ago

I think that makes sense, and is consistent with our current model.

	Zac C (:zac) Assignee
	Comment 15 • 10 years ago

I was thinking and I thought it might be nicer to have a MarionetteUnitTestCase that extends MarionetteTestCase.
The new class would do the Test Container switching and it'd avoid overloading MarionetteTestCase which is depended upon by Gaia and other test suites with no interest in the Test Container.

However I'll start work on the patch as per comment #13 as a starting point.

	Zac C (:zac) Assignee
	Comment 16 • 10 years ago

Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=4d7d186fad2a

	Jonathan Griffin (:jgriffin)
	Comment 18 • 10 years ago

Comment on attachment 8510352 [details] [diff] [review]
zac_bug_1075451.diff

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

Try results look ok, although Mnw is so busted it's a bit hard to tell.  We probably want a green run of Gip too.  Until bug 1087376 lands, you'll need to include in your try syntax: -u gaia-ui-test-accessibility,gaia-ui-test-unit,gaia-ui-test-functional-1,gaia-ui-test-functional-2,gaia-ui-test-functional-3

::: testing/marionette/client/marionette/marionette_test.py
@@ -464,5 @@
>          self.marionette.switch_to_frame(frame)
>  
>      def close_test_container(self):
> -        self.marionette.set_context("content")
> -        self.marionette.switch_to_frame()

Since we're removing this line, are we guaranteed to be in the right frame when we do this?  I guess so, from the existing call site.  Might be worth adding a comment that we assume this to be the case.

	Jonathan Griffin (:jgriffin)
	Comment 19 • 10 years ago

(In reply to Zac C (:zac) from comment #15)
> I was thinking and I thought it might be nicer to have a
> MarionetteUnitTestCase that extends MarionetteTestCase.
> The new class would do the Test Container switching and it'd avoid
> overloading MarionetteTestCase which is depended upon by Gaia and other test
> suites with no interest in the Test Container.
> 
> However I'll start work on the patch as per comment #13 as a starting point.

Ultimately, I'd like to disentangle the life cycles of tests, Marionette sessions, processes, and now, test-container apps.  I expect that to be a fair amount of work, so I think the current approach is a fine way forward in the short term.

	Zac C (:zac) Assignee
	Comment 20 • 10 years ago

(In reply to Jonathan Griffin (:jgriffin) from comment #18)
> Comment on attachment 8510352 [details] [diff] [review]
> zac_bug_1075451.diff
> 
> Review of attachment 8510352 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Try results look ok, although Mnw is so busted it's a bit hard to tell.  We
> probably want a green run of Gip too.  Until bug 1087376 lands, you'll need
> to include in your try syntax: -u
> gaia-ui-test-accessibility,gaia-ui-test-unit,gaia-ui-test-functional-1,gaia-
> ui-test-functional-2,gaia-ui-test-functional-3
> 

Thanks, that explains why Try wasn't running the Gip! (it had me confused)

> ::: testing/marionette/client/marionette/marionette_test.py
> @@ -464,5 @@
> >          self.marionette.switch_to_frame(frame)
> >  
> >      def close_test_container(self):
> > -        self.marionette.set_context("content")
> > -        self.marionette.switch_to_frame()
> 
> Since we're removing this line, are we guaranteed to be in the right frame
> when we do this?  I guess so, from the existing call site.  Might be worth
> adding a comment that we assume this to be the case.

In my experience yes switching context always goes to the top frame but on second thoughts I might put this line back in just to protect against that behaviour changing in the future.

	Zac C (:zac) Assignee
	Comment 21 • 10 years ago

Here's another Try:
https://tbpl.mozilla.org/?tree=Try&rev=a5a52790d76a

	Malini Das [:mdas] - Away, not checking bugmail
	Comment 22 • 10 years ago

Comment on attachment 8510352 [details] [diff] [review]
zac_bug_1075451.diff

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

lgtm, thanks!

	Zac C (:zac) Assignee
	Comment 23 • 10 years ago

Thanks

	Carsten Book [:Tomcat]
	Comment 24 • 10 years ago

Hey Zac, patch failed to apply cleanly:

patching file testing/marionette/client/marionette/runner/base.py
Hunk #4 FAILED at 825
1 out of 4 hunks FAILED -- saving rejects to file testing/marionette/client/marionette/runner/base.py.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir

could you take a look? thanks!

	Zac C (:zac) Assignee
	Comment 25 • 10 years ago

Tomcat thanks, this was applying to m-c but not to m-i.

This one should apply to m-i correctly now.

	Carsten Book [:Tomcat]
	Comment 26 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/f10c312a4bd9

	Ryan VanderMeulen [:RyanVM]
	Comment 27 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/f10c312a4bd9

	Carsten Book [:Tomcat]
	Comment 28 • 10 years ago

(In reply to Zac C (:zac) from comment #25)
> Created attachment 8512607 [details] [diff] [review]
> zac_bug_1075451_v2.diff
> 
> Tomcat thanks, this was applying to m-c but not to m-i.
> 
> This one should apply to m-i correctly now.

np and thanks :)

	Ryan VanderMeulen [:RyanVM]
	Comment 29 • 10 years ago

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/74e53479185e

	Ryan VanderMeulen [:RyanVM]
	Comment 30 • 10 years ago

Nothing in life is ever as simple as it seems. Backed out for permafail.
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/03d5e19bc3b4

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=31903&repo=mozilla-b2g34_v2_1

09:36:01 INFO - Traceback (most recent call last):
09:36:01 INFO - File "/builds/slave/test/build/tests/marionette/marionette/runtests.py", line 40, in <module>
09:36:01 INFO - cli()
09:36:01 INFO - File "/builds/slave/test/build/tests/marionette/marionette/runtests.py", line 35, in cli
09:36:01 INFO - runner = startTestRunner(runner_class, options, tests)
09:36:01 INFO - File "/builds/slave/test/build/tests/marionette/marionette/runtests.py", line 20, in startTestRunner
09:36:01 INFO - runner.run_tests(tests)
09:36:01 INFO - File "/builds/slave/test/build/tests/marionette/marionette/runner/base.py", line 699, in run_tests
09:36:01 INFO - self.run_test_sets()
09:36:01 INFO - File "/builds/slave/test/build/tests/marionette/marionette/runner/base.py", line 881, in run_test_sets
09:36:01 INFO - self.run_test_set(self.tests)
09:36:01 INFO - File "/builds/slave/test/build/tests/marionette/marionette/runner/base.py", line 862, in run_test_set
09:36:01 INFO - self.run_test(test['filepath'], test['expected'], test['test_container'])
09:36:01 INFO - File "/builds/slave/test/build/tests/marionette/marionette/runner/base.py", line 836, in run_test
09:36:01 INFO - self.launch_test_container()
09:36:01 INFO - File "/builds/slave/test/build/tests/marionette/marionette/runner/base.py", line 655, in launch_test_container
09:36:01 INFO - }""", script_timeout=60000)
09:36:01 INFO - File "/builds/slave/test/build/tests/marionette/marionette/marionette.py", line 1252, in execute_async_script
09:36:01 INFO - filename=os.path.basename(frame[0]))
09:36:01 INFO - File "/builds/slave/test/build/tests/marionette/marionette/decorators.py", line 35, in _
09:36:01 INFO - return func(*args, **kwargs)
09:36:01 INFO - File "/builds/slave/test/build/tests/marionette/marionette/marionette.py", line 638, in _send_message
09:36:01 INFO - self._handle_error(response)
09:36:01 INFO - File "/builds/slave/test/build/tests/marionette/marionette/marionette.py", line 700, in _handle_error
09:36:01 INFO - raise errors.ScriptTimeoutException(message=message, status=status, stacktrace=stacktrace)
09:36:01 INFO - errors.ScriptTimeoutException: ScriptTimeoutException: timed out
09:36:01 ERROR - Return code: 1