Closed Bug 1075451 Opened 5 years ago Closed 5 years ago

MarionetteTestCase tries to close test container in Gaia tests.

Categories

(Testing :: Marionette, defect)

x86_64
Linux
defect
Not set

Tracking

(firefox34 wontfix, firefox35 wontfix, firefox36 fixed, b2g-v2.1 affected, b2g-v2.2 fixed)

RESOLVED FIXED
mozilla36
Tracking Status
firefox34 --- wontfix
firefox35 --- wontfix
firefox36 --- fixed
b2g-v2.1 --- affected
b2g-v2.2 --- fixed

People

(Reporter: zcampbell, Assigned: zcampbell)

References

Details

Attachments

(2 files, 2 obsolete files)

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
Mdas or jgriffin, can you guys work on this as a follow-up to the bug that introduced this issue?
Flags: needinfo?(mdas)
Flags: needinfo?(jgriffin)
Summary: CommonTestCase tries to close test container in Gaia tests. → MarionetteTestCase tries to close test container in Gaia tests.
Blocks: 1073651
(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?
Flags: needinfo?(mdas)
I think that would be ok, unless we can just leave the test-container open and not kill it at all.
Flags: needinfo?(jgriffin)
(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
Attached patch test_con (obsolete) — Splinter Review
will mark for r? when try is green.

try: https://tbpl.mozilla.org/?tree=Try&rev=6df08297324a
Assignee: nobody → mdas
there's an Mn failure on emulator I can't reproduce locally. I have to rebuild my emulator. This is going to be fun.
Blocks: 1078156
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.
Attached patch bug_1075451.diff (obsolete) — Splinter Review
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
Attachment #8505443 - Flags: feedback?(mdas)
Attachment #8505443 - Flags: feedback?(dave.hunt)
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?
Attachment #8505443 - Flags: feedback?(dave.hunt) → feedback-
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.
Attachment #8505443 - Flags: feedback?(mdas)
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.
(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.
Flags: needinfo?(mdas)
Flags: needinfo?(jgriffin)
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.
Flags: needinfo?(mdas)
I think that makes sense, and is consistent with our current model.
Flags: needinfo?(jgriffin)
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.
Assignee: mdas → zcampbell
Attachment #8498385 - Attachment is obsolete: true
Attachment #8505443 - Attachment is obsolete: true
Attachment #8510352 - Flags: feedback?(mdas)
Attachment #8510352 - Flags: feedback?(jgriffin)
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.
Attachment #8510352 - Flags: feedback?(jgriffin) → feedback+
(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.
(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.
Comment on attachment 8510352 [details] [diff] [review]
zac_bug_1075451.diff

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

lgtm, thanks!
Attachment #8510352 - Flags: review+
Attachment #8510352 - Flags: feedback?(mdas)
Attachment #8510352 - Flags: feedback+
Thanks
Keywords: checkin-needed
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!
Flags: needinfo?(zcampbell)
Tomcat thanks, this was applying to m-c but not to m-i.

This one should apply to m-i correctly now.
Flags: needinfo?(zcampbell) → needinfo?(cbook)
https://hg.mozilla.org/mozilla-central/rev/f10c312a4bd9
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
(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 :)
Flags: needinfo?(cbook)
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
You need to log in before you can comment on or make changes to this bug.