1001326 - Split out GaiaApps into a file

Status: RESOLVED WONTFIX

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

Description

[Zac C (:zac)]

I'm going to experiment with moving GaiaApp/GaiaApps classes into a separate file.

We don't have to take it; we'll just float it for feedback from all.

Comment 1

[Zac C (:zac)]

Attachment: github pr

Step 1 on working on 987046.

Taking feedback for this.

Comment 2

[Florin Strugariu [:Bebe]]

Comment on attachment 8412448 [details] [review]
github pr

I like the idea but:

I think it would be a good idea to move the gaia_apps.py in a separate folder

If we go forward with this I see more parts of gaia_test.py moved out of the file. Having files in the gaiatest folder might be misleading and create confusion.

Comment 3

[Zac C (:zac)]

(In reply to Florin Strugariu [:Bebe] from comment #2)
> Comment on attachment 8412448 [details] [review]
> github pr
> 
> I like the idea but:
> 
> I think it would be a good idea to move the gaia_apps.py in a separate folder
> 
> If we go forward with this I see more parts of gaia_test.py moved out of the
> file. Having files in the gaiatest folder might be misleading and create
> confusion.

Yep! I'd like to move GaiaData class and others out too.

I hope we can start the dialogue in this bug to work out the naming, location, etc that we will use for the other classes.

Comment 4

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

Comment on attachment 8412448 [details] [review]
github pr

Note that there are packages that are currently using GaiaApps from gaiatest, so a change like this will need to be carefully managed so that it does not cause unexpected breakages.

See for example: https://github.com/mozilla/eideticker/blob/f82df6458b593c5abb3dde1b36511e0e75bff1b7/src/eideticker/eideticker/device.py#L15

Comment 5

[Zac C (:zac)]

We can try and put all these changes into just one gaiatest release to minimise the migration pain.

Comment 6

[Paul Yang [:pyang] (away)]

Comment on attachment 8412448 [details] [review]
github pr

Looks good

Comment 7

[Zac C (:zac)]

(In reply to Florin Strugariu [:Bebe] from comment #2)
> 
> I like the idea but:
> 
> I think it would be a good idea to move the gaia_apps.py in a separate folder
> 
> If we go forward with this I see more parts of gaia_test.py moved out of the
> file. Having files in the gaiatest folder might be misleading and create
> confusion.

Anybody else got some ideas/opinion on this?

I do agree a bit with Bebe but I don't know what folder to name it (or an existing folder?)

Comment 8

[Paul Yang [:pyang] (away)]

They're app toolkit, some are wrapper. umm..No good name come to me as well.

Comment 9

[Robert Wood [:rwood]]

Comment on attachment 8412448 [details] [review]
github pr

I think this is a great idea, makes the code cleaner and more manageable. A bit concerned about breaking existing clients though, as :davehunt mentioned.

Comment 10

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8412448 [details] [review]
github pr

I like it too. This is definitely something worth doing. Regarding backwards compatibility, we could take an approach where we keep an empty GaiaApps object in gaia_test.py which simply extends the new refactored object. That would allow any projects currently importing it directly to keep functioning without needing to be changed.

It's not ideal, but would address that pain point. If we don't want to do that then +1 to Zac's suggestion of doing one big refactor and putting it into a single release so that each project will have a chance to adjust before moving up to that release.

As for where to put the files, I'm not too fussed about having them in the same folder as gaia_test.py. If pressed for a name for a subfolder, I'd probably choose `modules`.

Comment 11

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

(In reply to Bob Silverberg [:bsilverberg] from comment #10)
> Comment on attachment 8412448 [details] [review]
> github pr
> 
> I like it too. This is definitely something worth doing. Regarding backwards
> compatibility, we could take an approach where we keep an empty GaiaApps
> object in gaia_test.py which simply extends the new refactored object. That
> would allow any projects currently importing it directly to keep functioning
> without needing to be changed.
> 
> It's not ideal, but would address that pain point. If we don't want to do
> that then +1 to Zac's suggestion of doing one big refactor and putting it
> into a single release so that each project will have a chance to adjust
> before moving up to that release.

I'm not against this approach, but we should keep in mind that some packages depend on whatever the latest gaiatest version on PyPI is. I would suggest raising bugs for each of eideticker, b2gpopulate, and b2gperf (those are certainly the main ones) so we can line up patches ahead of the gaiatest release that includes this refactoring. Note that this will mean all of these packages will then _require_ the latest gaiatest, and some of these currently need to run against v1.3, so we're in a catch-22... we will either need to commit to supporting v1.3 in the latest gaiatest or branch the various packages.

> As for where to put the files, I'm not too fussed about having them in the
> same folder as gaia_test.py. If pressed for a name for a subfolder, I'd
> probably choose `modules`.

I'm +1 for keeping them alongside gaia_test.py

Comment 12

[Zac C (:zac)]

Thanks Dave.

We also discussed offline and we'll start merging these now, before the next gaiatest release.

Now I'll put this up for r? from the usual suspects.

Comment 13

[Zac C (:zac)]

Comment on attachment 8412448 [details] [review]
github pr

now we sort of agreed to keep the structure generally the same, r? for functionality now :)

Comment 14

[Bob Silverberg [:bsilverberg]]

(In reply to Dave Hunt (:davehunt) from comment #11)
> (In reply to Bob Silverberg [:bsilverberg] from comment #10)
> > Comment on attachment 8412448 [details] [review]
> > github pr
> > 
> > I like it too. This is definitely something worth doing. Regarding backwards
> > compatibility, we could take an approach where we keep an empty GaiaApps
> > object in gaia_test.py which simply extends the new refactored object. That
> > would allow any projects currently importing it directly to keep functioning
> > without needing to be changed.
> > 
> > It's not ideal, but would address that pain point. If we don't want to do
> > that then +1 to Zac's suggestion of doing one big refactor and putting it
> > into a single release so that each project will have a chance to adjust
> > before moving up to that release.
> 
> I'm not against this approach, but we should keep in mind that some packages
> depend on whatever the latest gaiatest version on PyPI is. I would suggest
> raising bugs for each of eideticker, b2gpopulate, and b2gperf (those are
> certainly the main ones) so we can line up patches ahead of the gaiatest
> release that includes this refactoring. Note that this will mean all of
> these packages will then _require_ the latest gaiatest, and some of these
> currently need to run against v1.3, so we're in a catch-22... we will either
> need to commit to supporting v1.3 in the latest gaiatest or branch the
> various packages.
> 

I'm a bit confused. Wouldn't taking the approach I suggested, where we keep the existing API and just delegate to the new components, do away with the need to make any changes to those other projects? If we want them to continue to work on current gaiatest it sounds like we need to seriously consider that approach.

Comment 15

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

(In reply to Bob Silverberg [:bsilverberg] from comment #14)
> (In reply to Dave Hunt (:davehunt) from comment #11)
> > (In reply to Bob Silverberg [:bsilverberg] from comment #10)
> > > Comment on attachment 8412448 [details] [review]
> > > github pr
> > > 
> > > I like it too. This is definitely something worth doing. Regarding backwards
> > > compatibility, we could take an approach where we keep an empty GaiaApps
> > > object in gaia_test.py which simply extends the new refactored object. That
> > > would allow any projects currently importing it directly to keep functioning
> > > without needing to be changed.
> > > 
> > > It's not ideal, but would address that pain point. If we don't want to do
> > > that then +1 to Zac's suggestion of doing one big refactor and putting it
> > > into a single release so that each project will have a chance to adjust
> > > before moving up to that release.
> > 
> > I'm not against this approach, but we should keep in mind that some packages
> > depend on whatever the latest gaiatest version on PyPI is. I would suggest
> > raising bugs for each of eideticker, b2gpopulate, and b2gperf (those are
> > certainly the main ones) so we can line up patches ahead of the gaiatest
> > release that includes this refactoring. Note that this will mean all of
> > these packages will then _require_ the latest gaiatest, and some of these
> > currently need to run against v1.3, so we're in a catch-22... we will either
> > need to commit to supporting v1.3 in the latest gaiatest or branch the
> > various packages.
> > 
> 
> I'm a bit confused. Wouldn't taking the approach I suggested, where we keep
> the existing API and just delegate to the new components, do away with the
> need to make any changes to those other projects? If we want them to
> continue to work on current gaiatest it sounds like we need to seriously
> consider that approach.

Sorry Bob, we've been discussing this over e-mail and I think we're going to try pinning to branched versions of gaiatest. We'll see how this plays out in bug 1008234.

Comment 16

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

Comment on attachment 8412448 [details] [review]
github pr

Just a PEP8 nit.

Comment 17

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8412448 [details] [review]
github pr

LGTM and tests still pass locally.

Comment 18

[Zac C (:zac)]

I rebased this.

Dave, do you feel comfortable to start making these file changes to gaiatest?

Comment 19

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

I'm fine with these changes landing, but we need to manage the packages that consume gaiatest. We either need to continue to support the existing API (perhaps with a deprecation notice), or we need to raise bugs for the consumers. These bugs should then block a bug for the next release of gaiatest. This bug will at least affect the imports in b2gperf and eideticker.

Comment 20

[Zac C (:zac)]

WONTFIX, we all agreed this is low priority/reward relative to the difficulty/risk landing it.