Closed
Bug 841422
Opened 12 years ago
Closed 11 years ago
Move unit tests for shared code out of gallery
Categories
(Firefox OS Graveyard :: Gaia, defect)
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/
Assignee | ||
Comment 1•12 years ago
|
||
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.
Assignee | ||
Comment 2•12 years ago
|
||
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)
Comment 3•12 years ago
|
||
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)
Comment 4•12 years ago
|
||
I would personally opt for 2 then load the shared/ stuff in the system app domain
Assignee | ||
Comment 5•12 years ago
|
||
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)
Comment 6•11 years ago
|
||
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?
Comment 7•11 years ago
|
||
Rik, let's work together on this if you want :)
Comment 8•11 years ago
|
||
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 ]
Comment 9•11 years ago
|
||
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 :)
Comment 10•11 years ago
|
||
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 ]
Assignee | ||
Comment 11•11 years ago
|
||
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
Assignee | ||
Updated•11 years ago
|
Whiteboard: [p=2],[systemsfe]
Target Milestone: --- → 2.0 S1 (9may)
Assignee | ||
Comment 12•11 years ago
|
||
Comment on attachment 8417629 [details]
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)
Assignee | ||
Updated•11 years ago
|
Attachment #8417629 -
Flags: review?(felash)
Attachment #8417629 -
Flags: review?(dflanagan)
Attachment #8417629 -
Flags: review?(21)
Comment 13•11 years ago
|
||
Comment on attachment 8417629 [details]
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+
Assignee | ||
Updated•11 years ago
|
Attachment #8417629 -
Flags: review?(dflanagan)
Assignee | ||
Comment 14•11 years ago
|
||
Finally. GTFO tests!
https://github.com/mozilla-b2g/gaia/commit/b503736c1c15c363f57c9dd4d079ff58eb90b63d
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 15•11 years ago
|
||
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+
Assignee | ||
Comment 16•11 years ago
|
||
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 → ---
Assignee | ||
Comment 17•11 years ago
|
||
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
Comment 18•11 years ago
|
||
(In reply to Kevin Grandon :kgrandon from comment #12)
> Comment on attachment 8417629 [details]
> 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...
Comment 19•11 years ago
|
||
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).
Assignee | ||
Comment 20•11 years ago
|
||
Comment on attachment 8417726 [details]
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
Assignee | ||
Comment 21•11 years ago
|
||
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
Comment 22•11 years ago
|
||
Thanks Kevin, even if I'm not that happy with that patch, it's still better than the previous situation.
Comment 23•11 years ago
|
||
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
Comment 24•11 years ago
|
||
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?
Assignee | ||
Comment 25•11 years ago
|
||
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.
Assignee | ||
Comment 26•11 years ago
|
||
Assignee | ||
Comment 27•11 years ago
|
||
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.
Description
•