[marionette-apps] Support switching between main and inline activity app frames

RESOLVED FIXED

Status

Testing
JSMarionette
RESOLVED FIXED
3 years ago
8 months ago

People

(Reporter: azasypkin, Assigned: jlorenzo)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

When in test we run main app instance and then the same app is run as inline activity it's impossible to use client.apps.switchToApp to switch "context" between these two instances. See bug 1140339 to get more details about real use case.

Looks like it doesn't work because of the fact that we always associate the app user wants to switch to with the first-in-the-tree iframe only [1].

One of ideas I have in mind is to introduce smth like client.apps.switchToActivity that internally will use "#windows iframe[src*='x'][parentapp]" selector instead of "#windows iframe[src*='x']".

[1] https://github.com/mozilla-b2g/marionette-apps/blob/1fba000bd20e0b8729f263a739d4d4d82543e44b/lib/waitforapp.js#L35
(Reporter)

Comment 1

3 years ago
Created attachment 8573872 [details] [review]
GitHub pull request URL

Hey Kevin,

Here's WIP patch with the idea I had in mind. I haven't added any tests yet as I'd like to confirm this approach with you first and I'm not sure about all conventions you have in marionette-apps repo. I added some questions at GitHub as well.

It would be great to have your early feedback.

Thanks!
Attachment #8573872 - Flags: feedback?(kgrandon)
Comment on attachment 8573872 [details] [review]
GitHub pull request URL

I think the direction is fine, but probably :gaye or :lightsofapollo should have the final say. Please flag them for review when the time comes, thanks!
Attachment #8573872 - Flags: feedback?(kgrandon) → feedback+
(Reporter)

Comment 3

3 years ago
(In reply to Kevin Grandon :kgrandon from comment #2)
> Comment on attachment 8573872 [details] [review]
> GitHub pull request URL (wip)
> 
> I think the direction is fine, but probably :gaye or :lightsofapollo should
> have the final say. Please flag them for review when the time comes, thanks!

Sure, thanks for the feedback!
Assignee: nobody → azasypkin
Status: NEW → ASSIGNED
(Reporter)

Comment 4

3 years ago
Comment on attachment 8573872 [details] [review]
GitHub pull request URL

Hey Gareth,

Could you please review this patch? I've left several comments and questions about things that bother me at GitHub.

Thanks!
Attachment #8573872 - Attachment description: GitHub pull request URL (wip) → GitHub pull request URL
Attachment #8573872 - Flags: review?(gaye)
Overall I like the approach and you did really good work here :). I want to take a second closer look and think about the multiple windows thing a bit more though so will get back to this in a bit.
(Reporter)

Comment 6

3 years ago
(In reply to Gareth Aye [:gaye] from comment #5)
> Overall I like the approach and you did really good work here :). I want to
> take a second closer look and think about the multiple windows thing a bit
> more though so will get back to this in a bit.

Thanks Gareth, just in case you need example of integration test that may use switchToActivity - there is a WIP patch in bug 1140339.
(Reporter)

Comment 7

3 years ago
Hey Gareth,

Just wondering if you had a chance to take a closer look at the patch? :)

If you have any better idea, just let me know and I'll try to draft it out as well.

Thanks!
Flags: needinfo?(gaye)
(Assignee)

Updated

3 years ago
Depends on: 1170151
(Reporter)

Comment 8

3 years ago
Okay, since there is no any sign of interest in landing this bug for a such long period, removing assignee.
Assignee: azasypkin → nobody
Status: ASSIGNED → NEW
Hi! With NGA we will could have (potentially) different .html for handling different activities, which is really good in terms of performance (just load what you need), but collides with the way we are testing right now.

When testing an activity, currently we are using 'switchTo', which internally is using 'switchToApp' [1]. This function is waiting to load the App in [2] based on the source, and here is where the bug appears.
This source is the 'entrypoint' or 'launch_path' defined in the Manifest [3], but for activities this can not be the case (an activity could be handler by a different .html file). So, for example, in the case of #new activity in Contacts (based on New Gaia Architecture), bugs appear when the tests is trying to find an <iframe> with source '/contacts/index.html' (entrypoint), when the real path must be '/contacts/views/form/form.html' which is the handler of the activity.

I hope it helps!


[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/contacts/test/marionette/lib/contacts.js#L164

[2] https://github.com/mozilla-b2g/gaia/blob/master/tests/jsmarionette/plugins/marionette-apps/lib/switchtoapp.js#L26

[3] https://github.com/mozilla-b2g/gaia/blob/master/tests/jsmarionette/plugins/marionette-apps/lib/getapp.js#L81
Flags: needinfo?(jlorenzo)
Created attachment 8643123 [details] [review]
[gaia] JohanLorenzo:bug-1140344 > mozilla-b2g:master
(Assignee)

Updated

3 years ago
Assignee: nobody → jlorenzo
Flags: needinfo?(jlorenzo)
Flags: needinfo?(gaye)
(Assignee)

Comment 11

3 years ago
Comment on attachment 8643123 [details] [review]
[gaia] JohanLorenzo:bug-1140344 > mozilla-b2g:master

I reused Oleg's patch to reactivate the tests that were failing. The Gij tests are passing.

I wanted to add the unit tests Oleg has put, and another set for switchToActivity, but it seems they are totally ignored. What I do is:
> tests/jsmarionette/run_tests.js

Even if I change one test in the ones I added to make it fail, I don't see the failure. However, I have 3 failing:

>  390 passing (8s)
>  2 pending
>  3 failing
> 
>  1) run launch b2g:
>     Uncaught AssertionError: [ '-no-remote' ] deepEqual [ '-profile', undefined, '-no-remote' ]
> 
>  2) run launch b2g with different screen:
>     Uncaught AssertionError: [ '--screen=1280x800@160', '-no-remote' ] deepEqual [ '-profile', undefined, '--screen=1280x800@160', '-no-remote' ]
> 
>  3) TBPL #getFile - relative:

Gareth, are these errors known? How can I make the new test runs?
Flags: needinfo?(gaye)
(Assignee)

Comment 12

3 years ago
Comment on attachment 8573872 [details] [review]
GitHub pull request URL

The new patch is mostly the same. I make that one obsolete, just to make the attachment list cleaner.
Attachment #8573872 - Attachment is obsolete: true
Attachment #8573872 - Flags: review?(gaye)
I think I answered this question on irc today, but feel free to re-flag if you want me to review (or if you have another question).
Flags: needinfo?(gaye)
(Assignee)

Comment 14

3 years ago
Comment on attachment 8643123 [details] [review]
[gaia] JohanLorenzo:bug-1140344 > mozilla-b2g:master

Thanks for the help. For future readers, the command line to use to run these tests is:
> HARNESS_ONLY=1 make test-integration

I un-bitrot the tests given by Oleg. Which job on Treeherder should I take a look at, in order to make sure nothing is broken?
Attachment #8643123 - Flags: review?(gaye)
(Assignee)

Updated

3 years ago
See Also: → bug 1195230
(Assignee)

Comment 15

3 years ago
Comment on attachment 8643123 [details] [review]
[gaia] JohanLorenzo:bug-1140344 > mozilla-b2g:master

Gareth is on PTO at the moment. The NGA people would like this feature to re-enable some tests (and make sure no regression happens). Aus, would you mind taking a look?
Attachment #8643123 - Flags: review?(aus)

Comment 16

2 years ago
The marionette-apps changes seem sane to me. If you have a green run, I don't see why we shouldn't re-enable these! :)

Updated

2 years ago
Attachment #8643123 - Flags: review?(aus) → review+
(Assignee)

Comment 17

2 years ago
I got some conflicts. I fixed them and got a perfect green build on Gij[1]. Landing.

[1] https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=1ec05cb6eab2279532098ad769c19bc21fa4e0fe
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Hey Johan,

Have you finally landed this? I see PR is still not merged, but bug is marked as FIXED.

Thanks
Flags: needinfo?(jlorenzo)
(Assignee)

Comment 19

2 years ago
My bad, I thought I did the merge a month ago. I'm rebasing the patch due to the backout of Contacts NGA. Thanks for pointing this out.
Status: RESOLVED → REOPENED
Flags: needinfo?(jlorenzo)
Resolution: FIXED → ---
(Assignee)

Comment 20

2 years ago
I rebased the patch. Treeherder is green[1]. Landed in master at: https://github.com/mozilla-b2g/gaia/commit/8b80f85a2132dfacdffd30e1fb56ddd66eb45251 . Sorry for the inconvenience.

[1] https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=1ee6b4e3a7e74ed181945fb5a373e430b9cb7a5a
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
Resolution: --- → FIXED
Yay, thanks!
This has caused Mulet Gij(35) to turn intermittently orange.

See: https://treeherder.mozilla.org/#/jobs?repo=b2g-inbound&filter-searchStr=Mulet%20gij%2835%29&fromchange=6028872f957b&tochange=e51d0fed0d8d

The failures look like this:

https://treeherder.mozilla.org/logviewer.html#?job_id=2910358&repo=b2g-inbound

I'm going to revert this to fix the bustage.
Reverted  in https://github.com/mozilla-b2g/gaia/commit/709127219de9864211bc50017c58f563f1b9345e
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Created attachment 8671917 [details] [review]
[gaia] JohanLorenzo:bug-1140344-2 > mozilla-b2g:master
(Assignee)

Comment 25

2 years ago
The latest pull request still shows some intermittent-cy, but not orange to declare[1][2]. The reason of these intermittent failures is bug 1175080, which has had no update since July 13th (see 1175080#c60). We don't have update there, because we disabled tests that I re-enabled in my patch. 

[1] https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=0122c551ec17255f6f20d87bd4c253962d1f667c
[2] https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=608cdf1481ab7e3f26cdf3b9c6dea3f51d43ae8d
(Assignee)

Updated

2 years ago
Blocks: 1175080
(Assignee)

Comment 26

2 years ago
The changes I made between the first PR and the second remove the unclear error message "assert.fail()", in order to show the real problem: the connection to Marionette is lost.

@Sherrifs: If this PR gives more oranges, please, only deactivate the tests that are failing. Like said in comment 25, they are caused by bug 1175080, which is not related with the main work of this patch.

Landed in master at https://github.com/mozilla-b2g/gaia/commit/06a5b0bf1a6106a8d3f77aef6c42519061184d45
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
Resolution: --- → FIXED
I'm reasonably confident that the extra instances of bug 1175080 that we're seeing lately on Gij(35) is thanks to this push. Could you take a look?


Here's a few sample logs
https://treeherder.mozilla.org/logviewer.html#?job_id=15678358&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=15677348&repo=mozilla-inbound
Flags: needinfo?(jlorenzo)
Hey Johan,

after this patch I expected to be able to switch to windows opened using window.open, but after trying it seems I can't ? Should we file a separate bug ?
(Assignee)

Comment 29

2 years ago
(In reply to Nigel Babu [:nigelb] from comment #27)
Thanks for the heads up. They will be disabled in bug 1175080.

(In reply to Julien Wajsberg [:julienw] from comment #28)
I'm assuming this window.open() doesn't open an iFrame outside the app iFrame (like Contacts does when you go to the Gmail login page). So this is likely a bug. Could you open one with the code you have?
Flags: needinfo?(jlorenzo)
> I'm assuming this window.open() doesn't open an iFrame outside the app iFrame (like Contacts does when you go to the Gmail login page). So this is likely a bug. Could you open one with the code you have?

Actually I think it does :)
You can try by yourself by following the STR in 1213553.
Depends on: 1216464
I cherry-pick this commit to the NGA branch, so the guys working there can have the integration tests working:

https://github.com/mozilla-b2g/gaia/commit/dc2ac2943a08b0e41c3f373129693351b67ef4ac
(Assignee)

Comment 32

8 months ago
Comment on attachment 8643123 [details] [review]
[gaia] JohanLorenzo:bug-1140344 > mozilla-b2g:master

Patch has been landed since then
Attachment #8643123 - Flags: review?(gaye)
You need to log in before you can comment on or make changes to this bug.