Closed Bug 1001326 Opened 10 years ago Closed 10 years ago

Split out GaiaApps into a file

Categories

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

x86_64
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: zcampbell, Unassigned)

References

Details

Attachments

(1 file)

46 bytes, text/x-github-pull-request
bsilverberg
: review+
davehunt
: review+
Bebe
: review+
davehunt
: feedback+
bsilverberg
: feedback+
wachen
: feedback+
rwood
: feedback+
pyang
: feedback+
askeing
: feedback+
Bebe
: feedback+
Details | Review
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.
Attached file github pr
Step 1 on working on 987046.

Taking feedback for this.
Attachment #8412448 - Flags: feedback?(wachen)
Attachment #8412448 - Flags: feedback?(rwood)
Attachment #8412448 - Flags: feedback?(pyang)
Attachment #8412448 - Flags: feedback?(fyen)
Attachment #8412448 - Flags: feedback?(dave.hunt)
Attachment #8412448 - Flags: feedback?(bob.silverberg)
Attachment #8412448 - Flags: feedback?(florin.strugariu)
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.
Attachment #8412448 - Flags: feedback?(florin.strugariu) → feedback-
Attachment #8412448 - Flags: feedback- → feedback+
(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 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
Attachment #8412448 - Flags: feedback?(dave.hunt) → feedback+
We can try and put all these changes into just one gaiatest release to minimise the migration pain.
Attachment #8412448 - Flags: feedback?(fyen) → feedback+
Comment on attachment 8412448 [details] [review]
github pr

Looks good
Attachment #8412448 - Flags: feedback?(pyang) → feedback+
Attachment #8412448 - Flags: feedback?(wachen) → feedback+
(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?)
They're app toolkit, some are wrapper. umm..No good name come to me as well.
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.
Attachment #8412448 - Flags: feedback?(rwood) → feedback+
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`.
Attachment #8412448 - Flags: feedback?(bob.silverberg) → feedback+
(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
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 on attachment 8412448 [details] [review]
github pr

now we sort of agreed to keep the structure generally the same, r? for functionality now :)
Attachment #8412448 - Flags: review?(florin.strugariu)
Attachment #8412448 - Flags: review?(dave.hunt)
Attachment #8412448 - Flags: review?(bob.silverberg)
(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.
Depends on: 1008234
(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 on attachment 8412448 [details] [review]
github pr

Just a PEP8 nit.
Attachment #8412448 - Flags: review?(dave.hunt) → review+
Attachment #8412448 - Flags: review?(florin.strugariu) → review+
Comment on attachment 8412448 [details] [review]
github pr

LGTM and tests still pass locally.
Attachment #8412448 - Flags: review?(bob.silverberg) → review+
I rebased this.

Dave, do you feel comfortable to start making these file changes to gaiatest?
Flags: needinfo?(dave.hunt)
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.
Flags: needinfo?(dave.hunt)
WONTFIX, we all agreed this is low priority/reward relative to the difficulty/risk landing it.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.