Closed Bug 841422 Opened 7 years ago Closed 6 years ago

Move unit tests for shared code out of gallery

Categories

(Firefox OS Graveyard :: Gaia, defect)

x86
macOS
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
2.0 S1 (9may)

People

(Reporter: kgrandon, Assigned: kgrandon)

References

Details

(Whiteboard: [p=2],[systemsfe])

Attachments

(2 files, 1 obsolete file)

Currently most of the tests for shared libraries live inside of the gallery unit test folder. It doesn't really make sense here, so these should be moved, to a different location - or perhaps even a mock app. 

My recommended location would be: /shared/test/unit/
This bug should also include creating a method to run tests from the shared/ location. Ideally the tests should automatically be run in the test server whenever the source or test is saved.
Mike - I think you've been doing some work with our testing infrastructure. Would you have any insights into how to accomplish this, or any desire on taking it on?
Flags: needinfo?(mhenretty)
Kevin, I've looked into this and I can see a couple of options. We could create a mock app that represents all the shared code, or we could change how the config file (which specifies all the unit tests) gets generated to include the location where the shared code is kept. On first glance, I like the second option, but I could be missing a lot of info here so I'm open to suggestion.

Also, I wouldn't mind taking this on.
Flags: needinfo?(mhenretty) → needinfo?(kgrandon)
I would personally opt for 2 then load the shared/ stuff in the system app domain
There's been recent talk on moving shared code out of gaia, which Isupport. Let's hold off on this for now until we figure out our approach there.
Flags: needinfo?(kgrandon)
We're getting more and more code into shared these days. Having the tests located in a totally random is getting painful. Can we move forward here?
Rik, let's work together on this if you want :)
I am in the process of adding code to shared for bug 910876, so I can take a look at this as well.
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
Whiteboard: [ c= p=2 s=2013.09.06 ]
I think we should use a template app with a template origin to unit test the shared libraries.

but anything that works works for me :)
An added problem here is that the test-agent config file at:

  https://github.com/wanderview/gaia/blob/master/tools/test-agent/test-agent-server.js

Only accepts a single string for the path property.  This means we cannot point the server at both /apps and /shared, for example.  This is problematic because I assume we don't want an /apps/shared directory because it would be at risk of getting sucked into the overall build.

So I think test-agent proper needs to be enhanced here.

Unfortunately, this is more work than I originally thought.  Given my other sprint commitments this week I don't think I will have time.  I'm still happy to help in the future, but unassigning for right now in case someone else is able to take it.  Sorry for the bug churn!
Assignee: bkelly → nobody
Status: ASSIGNED → NEW
Whiteboard: [ c= p=2 s=2013.09.06 ]
Attached file Github pull request
I'm going to suggest that we take the easy route here and create a new app for unit tests for shared/ code.
Assignee: nobody → kgrandon
Status: NEW → ASSIGNED
Whiteboard: [p=2],[systemsfe]
Target Milestone: --- → 2.0 S1 (9may)
Comment on attachment 8417629 [details] [review]
Github pull request

Hey guys - just looking for a review here if anyone has time. It's not the ideal fix, but having these in the gallery app is really confusing/messy. Also splitting them out will allow us easily make a better fix in the future if these tests are in the same place. Thanks!
Attachment #8417629 - Flags: review?(jlal)
Attachment #8417629 - Flags: review?(felash)
Attachment #8417629 - Flags: review?(21)
Attachment #8417629 - Flags: review?(felash)
Attachment #8417629 - Flags: review?(dflanagan)
Attachment #8417629 - Flags: review?(21)
Comment on attachment 8417629 [details] [review]
Github pull request

I implicitly trust whatever black magic was done here to move the unrelated tests from gallery rs+.
Attachment #8417629 - Flags: review?(jlal) → review+
Attachment #8417629 - Flags: review?(dflanagan)
Finally. GTFO tests!

https://github.com/mozilla-b2g/gaia/commit/b503736c1c15c363f57c9dd4d079ff58eb90b63d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Attached file Follow-up, move sharedtest to dev_apps (obsolete) —
I get a retrospective R- on the previous patch for placing it in the apps folder and having it get packaged because of: https://github.com/mozilla-b2g/gaia/blob/master/build/config/phone/apps-production.list#L1

This is not terrible as it's hidden, but it does mean a slightly larger binary size. Moving this over to dev_apps for now, sorry for the churn on this. Got a verbal approval from lightsofapollo on this over IRC so placing that here.
Attachment #8417726 - Flags: review+
Argh, I'm going to reopen this until I have a good solution for the app not being installed: https://github.com/mozilla-b2g/gaia/commit/d902ffefa2aa4ad4e3cc987d90a532dcc95d8228
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I tried moving this into dev_apps but it also seems like dev_apps has some problems with the test agent. Seems like we should probably whitelist production apps as we will need to when apps for stringray hit: https://github.com/mozilla-b2g/gaia/blob/master/build/config/phone/apps-production.list
(In reply to Kevin Grandon :kgrandon from comment #12)
> Comment on attachment 8417629 [details] [review]
> Github pull request
> 
> Hey guys - just looking for a review here if anyone has time. It's not the
> ideal fix, but having these in the gallery app is really confusing/messy.
> Also splitting them out will allow us easily make a better fix in the future
> if these tests are in the same place. Thanks!

Thanks for doing that. It kind of makes me sad every time I had to edit one of those files...
Sorry for not coming here earlier.

The ideal solution is IMO loading an empty template provided by the test-agent for such tests. Maybe we can remove the need to add _proxy.html and other files in the apps themselves and load them on the fly?

We also need to watch also the shared tests (probably not difficult).
Blocks: 1006600
Comment on attachment 8417726 [details] [review]
Follow-up, move sharedtest to dev_apps

Ok, so we have bug 1006236 landed which whitelists apps for production build. The reason this landed is because for some reason the test-agent is not playing nicely with the dev_apps folder. I am now going to land the original patch and have filed two bugs against the test agent component. 

bug 1006599 for supporting unit tests in dev_apps/
bug 1006600 for watching files in shared/
Attachment #8417726 - Attachment is obsolete: true
Re-landed: https://github.com/mozilla-b2g/gaia/commit/1ca9e051b540708133fdd6c447337fe183dcc497
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Thanks Kevin, even if I'm not that happy with that patch, it's still better than the previous situation.
I think we had some tests for shared in other apps too (for example [1]). We'll need to move them too.

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/contacts/test/unit/contact2vcard_test.js
LazyL10n would be another example: https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/dialer/test/unit/lazy_l10n_test.js.  Not sure if this is the right place to mention this.  Should I file a new bug?
I'm doing a follow-up now for the contacts vcard, and will include the lazy_l10n in that pull request if it's ok with you? I'll just check them in against this bug.
Landed a follow-up to move some remaining tests into the sharedtest folder: https://github.com/mozilla-b2g/gaia/commit/870a5c518742665d36b17e7e88c2ab07d440b94c

If you guys see anything else please file a new bug. Thanks!
You need to log in before you can comment on or make changes to this bug.