841422 - Move unit tests for shared code out of gallery

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Description

[Kevin Grandon :kgrandon]

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/

Comment 1

[Kevin Grandon :kgrandon]

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.

Comment 2

[Kevin Grandon :kgrandon]

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?

Comment 3

[Michael Henretty [:mikehenrty][: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.

Comment 4

[James Lal [:lightsofapollo]]

I would personally opt for 2 then load the shared/ stuff in the system app domain

Comment 5

[Kevin Grandon :kgrandon]

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.

Comment 6

[Anthony Ricaud (:rik)]

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

[Julien Wajsberg [:julienw]]

Rik, let's work together on this if you want :)

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

I am in the process of adding code to shared for bug 910876, so I can take a look at this as well.

Comment 9

[Julien Wajsberg [:julienw]]

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

[Ben Kelly [:bkelly, not reviewing]]

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!

Comment 11

[Kevin Grandon :kgrandon]

Attachment: 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.

Comment 12

[Kevin Grandon :kgrandon]

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!

Comment 13

[James Lal [:lightsofapollo]]

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+.

Comment 14

[Kevin Grandon :kgrandon]

Finally. GTFO tests!

https://github.com/mozilla-b2g/gaia/commit/b503736c1c15c363f57c9dd4d079ff58eb90b63d

Comment 15

[Kevin Grandon :kgrandon]

Attachment: Follow-up, move sharedtest to dev_apps

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.

Comment 16

[Kevin Grandon :kgrandon]

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

Comment 17

[Kevin Grandon :kgrandon]

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

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(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...

Comment 19

[Julien Wajsberg [:julienw]]

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).

Comment 20

[Kevin Grandon :kgrandon]

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/

Comment 21

[Kevin Grandon :kgrandon]

Re-landed: https://github.com/mozilla-b2g/gaia/commit/1ca9e051b540708133fdd6c447337fe183dcc497

Comment 22

[Julien Wajsberg [:julienw]]

Thanks Kevin, even if I'm not that happy with that patch, it's still better than the previous situation.

Comment 23

[Julien Wajsberg [:julienw]]

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

[Staś Małolepszy :stas]

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?

Comment 25

[Kevin Grandon :kgrandon]

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.

Comment 26

[Kevin Grandon :kgrandon]

Attachment: Follow-up, move additional tests to sharedtest app

Comment 27

[Kevin Grandon :kgrandon]

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!