1000334 - Adjust b2g_start to work for v1.3 as well

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: Gaia::UI Tests, defect)

Description

[Bob Silverberg [:bsilverberg]]

In order to facilitate code for multiple versions of gaia, add a property to the GaiaData class that returns the gaia version.

Comment 1

[Bob Silverberg [:bsilverberg]]

Attachment: Pull request

Comment 2

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8411214 [details] [review]
Pull request

This doesn't seem to be working where I need it. I'll work on a new PR.

Comment 3

[Zac C (:zac)]

I worry about what crazy ways we'll use this to try and facilitate backwards dependency. 

I've already been going to some effort to remove loops of dependencies between the various classes so I'd rather not put it all back in.

Are you still sure you're going about all of this the right way?

Comment 4

[Bob Silverberg [:bsilverberg]]

I know this is a contentious issue, but Dave has been putting in fixes to gaiatest in order to stay on the latest version for b2gperf and b2gpopulate when needing to run those on v1.3, and I think that that is the best route to take. marketplace-tests-gaia could very well continue to run on v1.3 for quite some time, and having it on the current gaiatest will allow it to inherit all of the fixes and tweaks that gaiatest gets.

I decided it was worth at least a bit of investigation into whether I can get the current gaiatest working with v1.3 for marketplace-tests-gaia and I have found that I could with just a couple of small changes (one of them for the marionette test runner), so I do think it's worthwhile continuing to pursue this.

The fact that the a-team have committed (I believe) to making sure that the current version of marionette will work with v1.3 is another indication that this is a worthwhile path to take.

I have it all working now, with the only issue being that I sometimes see the old bug we encountered where the FTU splash screen stays displayed and the test runs behind it. I can address that quite easily by having a separate code path for v1.3 in b2g_start.

Because of complications getting it to work, and also to eliminate as much coupling as I can, I have decided against creating a gaia_version property in GaiaData because the only place I need it is in GaiaDevice. I have added the code to GaiaDevice to do this and am currently testing. I will submit a PR for that when it's ready, and then I will update this bug.

Comment 5

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Bob Silverberg [:bsilverberg] from comment #4)
> Because of complications getting it to work, and also to eliminate as much
> coupling as I can, I have decided against creating a gaia_version property
> in GaiaData because the only place I need it is in GaiaDevice. I have added
> the code to GaiaDevice to do this and am currently testing. I will submit a
> PR for that when it's ready, and then I will update this bug.

Sounds good to me. I think it would be helpful to add gaia_version to mozversion too, but not sure that would help in this situation as we'd return the full string, which would still require manipulation to allow version comparisons.

I also think that we should determine paths based on features rather than versions wherever possible. In the case of this wait, I suspect it may be difficult to do this, but it would certainly be worth a short investigation.

Comment 6

[Zac C (:zac)]

The feature difference here is WindowManager (v1.3), AppWindowManager (>v1.3).

Comment 7

[Bob Silverberg [:bsilverberg]]

Thanks Zac, that works well. PR coming soon that uses AppWindowManager.

Comment 8

[Bob Silverberg [:bsilverberg]]

Attachment: Pull request

Comment 9

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 8412615 [details] [review]
Pull request

Looks good, especially that we're able to avoid parsing the gaia version, but I think it could be cleaned up a little before we land it.

Comment 10

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8412615 [details] [review]
Pull request

Unfortunately it didn't work on Travis (a problem with desktop I believe), and also Zac pointed out that having that execute_script in there will open us up to those "harmless" Javascript exceptions being caught by marionette during startup which will kill the tests. I have pushed a new version which, unfortunately, does rely on parsing the gaia version.

Let's see what you think about this one.

Comment 11

[Zac C (:zac)]

Comment on attachment 8412615 [details] [review]
Pull request

I dislike this but we seem unable to reach a better compromise aside from promising ourselves we will rip it out the moment it's not needed.

Functionally it's OK.

Comment 12

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 8412615 [details] [review]
Pull request

Thanks Bob, it's a shame we can't do this another way. Somewhat related, I've raised bug 1002518 to add this to mozversion, however it wouldn't really change the complexity of the code you have in the patch as it is, and I wouldn't want to introduce a blocker.

Comment 13

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8412615 [details] [review]
Pull request

Thanks for the reviews, gents. Landed in https://github.com/mozilla-b2g/gaia/commit/e1993d7aa73c8ee093cfcfaf63390422e099e467

Comment 14

[Zac C (:zac)]

Had to back this out as it caused intermittents on Travis and TBPL:

https://tbpl.mozilla.org/?rev=e92f67d56b36&tree=B2g-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=38690326&tree=Mozilla-Inbound
etc

backout commit:
https://github.com/mozilla-b2g/gaia/commit/d0650e430b0557ec2cd9abf62bdac05ee96872ea

Comment 15

[Bob Silverberg [:bsilverberg]]

We came up with a better solution to this (thanks to Zac) which will exist in the marketplace-tests-gaia repo instead of gaiatest. This can be closed.