Closed Bug 1140344 Opened 5 years ago Closed 4 years ago

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

Categories

(Testing Graveyard :: JSMarionette, defect)

x86_64
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: azasypkin, Assigned: jlorenzo)

References

Details

Attachments

(2 files, 1 obsolete file)

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
Attached file GitHub pull request URL (obsolete) —
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+
(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
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.
(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.
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)
Depends on: 1170151
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)
Assignee: nobody → jlorenzo
Flags: needinfo?(jlorenzo)
Flags: needinfo?(gaye)
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)
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)
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)
See Also: → 1195230
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)
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! :)
Attachment #8643123 - Flags: review?(aus) → review+
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
Closed: 4 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)
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 → ---
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
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Yay, thanks!
Reverted  in https://github.com/mozilla-b2g/gaia/commit/709127219de9864211bc50017c58f563f1b9345e
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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
Blocks: 1175080
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
Closed: 4 years ago4 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 ?
(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
Comment on attachment 8643123 [details] [review]
[gaia] JohanLorenzo:bug-1140344 > mozilla-b2g:master

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