1078739 - [marionette-apps] Calling marionette-apps.list times out

Status: RESOLVED FIXED

(Testing Graveyard :: JSMarionette, defect)

Description

[Gareth Aye [:gaye] (back from PTO)]

Dale ran into a problem while writing a search test where launching the search app after closing it once previously is never finishing. It looks as if this script https://github.com/mozilla-b2g/marionette-apps/blob/master/lib/mgmt.js#L37 is never sending back a response.

Comment 1

[Gareth Aye [:gaye] (back from PTO)]

He called client.switchToFrame() immediately before calling app.launch, so navigator.mozApps.getAll should be fine AFAICT.

Comment 2

[Gareth Aye [:gaye] (back from PTO)]

s/navigator.mozApps.getAll/navigator.mozApps.mgmt.getAll/

Comment 3

[Gareth Aye [:gaye] (back from PTO)]

Also dale added this pr as a test case https://github.com/mozilla-b2g/gaia/pull/24848

Comment 4

[Dale Harvey (:daleharvey)]

So I debugged this a little, In this test I open the rocketbar and enter a url, this will open a background instance of the search application, I then try and launch a foreground instance of the search app

https://github.com/mozilla-b2g/marionette-apps/blob/master/lib/waitforapp.js#L35

does a blanket search for iframe[src=*="app://search.gaiamobile.org"] so I expect it is picking up the background search window, which is always hidden and will never return from that

Not sure what the best fix for this would be, CC'ing Kevin in case you have any ideas

Comment 5

[Kevin Grandon :kgrandon]

I'd need to dig into the code a bit, but I bet changing the selector to include some additional attributes might work. It's a bit tricky because both search windows are proper app windows, so it might make the selector a but more fragile, but something we have to live with.

The next two days or so I'm fairly slammed, but I wouldn't mind picking this up, so doing a ni? on myself to remind me to look into this if no one takes it.

Comment 6

[Dale Harvey (:daleharvey)]

If I change 

  var el = client.findElement('iframe[src*="' + source + '"]:not(.hidden)');

to 

  var el = client.findElement('iframe[src*="' + source + '"]');

It fixes my test, given the semantics it seems like it shouldnt break anything, but would appreciate any input on this and help getting it landed as its blocking a blocker

Comment 7

[Kevin Grandon :kgrandon]

(In reply to Dale Harvey (:daleharvey) from comment #6)
> If I change 
> 
>   var el = client.findElement('iframe[src*="' + source + '"]:not(.hidden)');
> 
> to 
> 
>   var el = client.findElement('iframe[src*="' + source + '"]');
> 
> It fixes my test, given the semantics it seems like it shouldnt break
> anything, but would appreciate any input on this and help getting it landed
> as its blocking a blocker

What about checking on '.active' or #windows iframe, using the #windows as a root selector?

Comment 8

[Kevin Grandon :kgrandon]

I'm fine with one of the fixes in Comment 6 or Comment 7. Dale - Let me know if I can help you write a patch if you are slammed or anything.

Comment 9

[Dale Harvey (:daleharvey)]

Its cool I have the time, I think changing the root selector would be a better option but for now this doesnt block the blocker since the changing requirements have changed the test so its not as urgent

Comment 10

[Dale Harvey (:daleharvey)]

Attachment: https://github.com/mozilla-b2g/marionette-apps/pull/39

This suggestion from kevin works for me and seems like it would have the least impact

Comment 11

[Gareth Aye [:gaye] (back from PTO)]

Comment on attachment 8502412 [details] [review]
https://github.com/mozilla-b2g/marionette-apps/pull/39

Sounds perfect. Landed on master https://github.com/mozilla-b2g/marionette-apps/commit/f100982d4f8f1d6b071aef62a8582d021e126989. You have to do some finagling with gaia-node-modules to downstream this to gaia (I know you already know that though : ). Thanks for working through this!!

Comment 12

[Dale Harvey (:daleharvey)]

Hey, Asking in IRC but guessing it was missed, could (preferably) someone publish this, or add me to the authors on npm so I can get this merged

Comment 13

[Kevin Grandon :kgrandon]

I've added you to marionette-apps.

Comment 14

[Dale Harvey (:daleharvey)]

Great thanks, can I get an example of how to actually test this, trying to figure out via the code and its not that clear between gaia_node_modules.revision and NODE_MODULES_GIT_URL / NODE_MODULES_SRC etc etc

I have published the update to marionette apps, and pushed a node_modules branch @ https://github.com/daleharvey/gaia-node-modules/tree/test-1078739

Comment 15

[Kevin Grandon :kgrandon]

I think you need to update the Makefile tarball path here: https://github.com/mozilla-b2g/gaia/blob/master/Makefile#L686 The revision should also be the same as in your branch.

Also you could possibly just land in gaia-node-modules master, and merge up to gaia first since you already got an R+.

Comment 16

[Dale Harvey (:daleharvey)]

Merged in https://github.com/mozilla-b2g/gaia/commit/6aa47bae89b9930527fd06b3eaeeb052757cb3d6

I had spent all day doing test runs to get this green bug kept hitting other bugs, cheers kevin