1006357 - make test agent to run tests from all apps locally and in travis

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Build, defect)

Description

[John Hu [:johnhu][:johu][:醬糊小弟]]

Stingray is the TV project which utilize GAIA_DEVICE_TYPE=tv as its build configuration. It contains an app called homescreen-stingray which is a homescreen support widgets at dev_apps.

During the development of homescreen-stingray, we found an app only existed in tv build instead of phone build. The test agent will get stuck at the end of unit test.

Test Agent got stuck with the following cases:
1. if we run: make test-agent-test APP=non-existed-app
   output:
Running tests for non-existed-app
./node_modules/test-agent/bin/js-test-agent test  --server ws://localhost:8789 --reporter spec
  <got stuck here>

2. if we run homescreen-stingray's tests with phone's build: make test-agent-test APP=homescreen-stingray
   output:
Running tests for homescreen-stingray
./node_modules/test-agent/bin/js-test-agent test  --server ws://localhost:8789 --reporter spec dev_apps/homescreen-stingray/test/unit/layout_editor_test.js
  <got stuck here>

3. if we run homescreen-stingray's tests with tv's build, make DEBUG=1 GAIA_DEVICE_TYPE=tv: make test-agent-test APP=homescreen-stingray
   everything works.

The same issue also happened in travis.

Comment 1

[John Hu [:johnhu][:johu][:醬糊小弟]]

(In reply to John Hu [:johnhu][:johu][:醬糊小弟] from comment #0)
> During the development of homescreen-stingray, we found an app only existed
> in tv build instead of phone build. The test agent will get stuck at the end
> of unit test.

It's a little hard to understand the above.

The problem is that: if an app only existed in non-phone build, in our case is tv build, the test agent will get stuck at the end of unit test and it tries to run the tests of this app which is not in profile-debug.

Comment 2

[Julien Wajsberg [:julienw]]

Does it happen if you regenerate your profile?

Comment 3

[John Hu [:johnhu][:johu][:醬糊小弟]]

Yes, it always happens.

Comment 4

[John Hu [:johnhu][:johu][:醬糊小弟]]

STR:

1. download the PR: https://github.com/mozilla-b2g/gaia/pull/18932
2. rename the dev_apps/homescreen-stingray/_test_discard to test
3. make clean
4. DEBUG=1 make and start test agent
5. make test-agent-test APP=homescreen-stingray

Comment 5

[John Hu [:johnhu][:johu][:醬糊小弟]]

The httpd doesn't add virtual folders to the folders which exists in source tree but not exists in the app.list.

Comment 6

[John Hu [:johnhu][:johu][:醬糊小弟]]

We will file another bug to trace no error message issue and use this bug to make all folders in apps and dev_apps be mapped to httpd.

So, all tests will be run by test agent, no matter it is built into profile or not.

Comment 7

[John Hu [:johnhu][:johu][:醬糊小弟]]

Attachment: patch for this bug

Wait for travis's result before setting review flag.

Comment 8

[Julien Wajsberg [:julienw]]

(In reply to John Hu [:johnhu][:johu][:醬糊小弟] from comment #6)
> We will file another bug to trace no error message issue 

I think you want bug 1006962 :)

Comment 9

[Julien Wajsberg [:julienw]]

(In reply to Julien Wajsberg [:julienw] (away May 8th) from comment #8)
> (In reply to John Hu [:johnhu][:johu][:醬糊小弟] from comment #6)
> > We will file another bug to trace no error message issue 
> 
> I think you want bug 1006962 :)

Oops, you actually created it, sorry ;)


What do you think og bug 1006599, is it the same than this bug?

Comment 10

[John Hu [:johnhu][:johu][:醬糊小弟]]

Maybe, we need to ask Kevin for more information...

Comment 11

[John Hu [:johnhu][:johu][:醬糊小弟]]

(In reply to Julien Wajsberg [:julienw] (away May 8th) from comment #9)
> What do you think og bug 1006599, is it the same than this bug?

I think they are the same. But we need to wait for Kevin's information.

Comment 12

[Kevin Grandon :kgrandon]

But does this work locally? The problem I was seeing is that the tests for dev_apps were also timing out locally for me.

Comment 13

[John Hu [:johnhu][:johu][:醬糊小弟]]

Comment on attachment 8418504 [details] [review]
patch for this bug

Yuren,

This patch is trying to build all apps in travis.

Comment 14

[John Hu [:johnhu][:johu][:醬糊小弟]]

(In reply to Kevin Grandon :kgrandon from comment #12)
> But does this work locally? The problem I was seeing is that the tests for
> dev_apps were also timing out locally for me.

Kevin,

This patch doesn't work locally. It only patches the script of travis to run all tests in our code base. If we want to run locally, we need to put the app to one of the list and build those apps to profile_debug. The key of this issue is that we don't add virtual folder to httpd. And the list of virtual folder is exactly the same as the app list which will be compiled to profile_debug.

Comment 15

[John Hu [:johnhu][:johu][:醬糊小弟]]

Travis is all green.

Comment 16

[John Hu [:johnhu][:johu][:醬糊小弟]]

Comment on attachment 8418504 [details] [review]
patch for this bug

As per offline discussion, we should solve this issue on both local or travis. 

We may need to decide whether or not test agent runs all tests even those tests are not built to profile.

Comment 17

[John Hu [:johnhu][:johu][:醬糊小弟]]

To workaround this issue, we will disable tests in homescreen-stingray.

Comment 18

[John Hu [:johnhu][:johu][:醬糊小弟]]

Julien and Kevin,

May we know your opinions about comment 16? Does test agent need to run all tests even if those tests are not built into profile?

Comment 19

[Julien Wajsberg [:julienw]]

I think it should; for example, the test-agent itself has unit tests.

Now, I'm somewhat lost in the recent build changes, so I don't exactly know what needs to be done. IIRC httpd.js was able to look into apps/ and test_apps/ and other directories before the change to the build_stage directory, but I don't know what changed.

Yuren will know better than me here.

Comment 20

[Kevin Grandon :kgrandon]

Yes, we should be able to run tests for apps in dev_apps locally. I am fine with running tests for all apps in this folder, as if there's a test there - it's probably important. If we are going to fix this for locally running tests, please rename the bug and dupe bug 1006599 against this. Thanks!

Comment 21

[John Hu [:johnhu][:johu][:醬糊小弟]]

Thanks for your information. I may take this issue when we have a final decision.

Comment 22

[John Hu [:johnhu][:johu][:醬糊小弟]]

(In reply to Julien Wajsberg [:julienw] (away May 8th) from comment #19)
> Now, I'm somewhat lost in the recent build changes, so I don't exactly know
> what needs to be done. IIRC httpd.js was able to look into apps/ and
> test_apps/ and other directories before the change to the build_stage
> directory, but I don't know what changed.

The symptom I found is that httpd.js only mounts the built apps on gaiamobile.org domain. For other apps, we cannot use http://xxx.gaiamobile.org:8080/ to access it, neither does test agent.

Comment 23

[Ricky Chien [:rickychien] (inactive)]

Hi, I think it would be a architecture problem.

In [1], "make test-agent-test" will find all tests in both apps/ and dev_apps/.

In [2], when we build gaia by using "make DEBUG=1 xxx", we also trigger this "test-agent-config" command to generate a config.json for test agent web app. So test agent web app can list tests after you built. But in here the search dir is different with [1]. It uses GAIA_APPDIRS which is decided from your build parameter in "make test agent xxx". So it's possible to be a problem because there are not synchronous.
On the other hand, httpd.js also take GAIA_APPDIRS as parameter to do URL routing. We only can open apps correctly that was built.

So if we launch "make test-agent-test APP=some-app-exist-in-apps-or-dev_apps-but-not-exist-in-GAIA_APPDIRS", Firefox couldn't access this app properly because httpd.js doesn't know.

I prefer to make a change in the [1] to read the GAIA_APPDIRS and let us test agent architecture more straightforward.
This solution is easier than patch httpd.js for searching both apps/ and dev_apps/.

[1] https://github.com/mozilla-b2g/gaia/blob/38b81f8e017df32479cbae07f7ee415a78984d8b/Makefile#L858
[2] https://github.com/mozilla-b2g/gaia/blob/38b81f8e017df32479cbae07f7ee415a78984d8b/Makefile#L834

Comment 24

[Ricky Chien [:rickychien] (inactive)]

This solution will break benefits of running all tests in both apps and dev_apps. However, there might be a developing trouble in the future if there exist two different searching rule in test agent. So we need to find a good tradeoff for this.

So I need your information here to make this changes.

Comment 25

[Kevin Grandon :kgrandon]

I'm not sure I can be too useful as I don't know this code off the top of my head. I thought that there was no way avoiding a httpd.js change here as we don't have a domain for the app to run tests on. 

Don't we need to have a working domain to run the tests?

Comment 26

[John Hu [:johnhu][:johu][:醬糊小弟]]

IMHO, I prefer to run tests who are already built into profile folder. This is because marionette tests, gaia ui tests do the same thing.

It may be confusing that we run all unit tests when we have really large code base or each app has its owned repo.

Comment 27

[John Hu [:johnhu][:johu][:醬糊小弟]]

(In reply to Kevin Grandon :kgrandon from comment #25)
> Don't we need to have a working domain to run the tests?

Yes, we need a domain to run them. Ricky's comment 23 is trying to change the code of httpd.js to create the domains for all apps.

Comment 28

[Ricky Chien [:rickychien] (inactive)]

Oh, I meant I do prefer to change the APP_TEST_LIST parameter to GAIA_APPDIRS in [1] instead of changing the code of httpd.js. For doing this, we can read this config.json in [2] which was generated in "make DEBUG=1" so that both test agent app and test agent server can use same searching rule.

[1] https://github.com/mozilla-b2g/gaia/blob/38b81f8e017df32479cbae07f7ee415a78984d8b/Makefile#L858
[2] https://github.com/mozilla-b2g/gaia/blob/38b81f8e017df32479cbae07f7ee415a78984d8b/Makefile#L828-L848

Comment 29

[Julien Wajsberg [:julienw]]

I don't say I understand everything here but I think I follow you here Ricky.

I think httpd.js can take whatever is in the correct pref and find the directory, right?

Comment 30

[Ricky Chien [:rickychien] (inactive)]

httpd.js can only resolve the domain in the correct pref so cause test agent iframe get stuck with "Unable access to homescreen-stingray.gaiamobile.org:8080" network error as well as a timeout on Travis.

We would see a "cannot access xxx" error on Travis after bug 1006962 is landed.

Comment 31

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

for two options:

1) test all apps: we need to patch bootstrap.js in httpd extension[1] and test-agent-config[2] to support scanning all test script in apps/ and dev_apps/.
2) test apps which are built: need to patch Makefile[3] which Ricky mentioned on comment 23

I recommand to implment option 1 to test all apps because unit tests should be run for all test scripts unless we specifiy an app.

[1] https://github.com/mozilla-b2g/gaia/blob/e8a1eedb848382323254edbd234baecebc5dc5ca/tools/extensions/httpd/bootstrap.js#L76-L85

[2] https://github.com/mozilla-b2g/gaia/blob/38b81f8e017df32479cbae07f7ee415a78984d8b/Makefile#L834

[3] https://github.com/mozilla-b2g/gaia/blob/38b81f8e017df32479cbae07f7ee415a78984d8b/Makefile#L858

Comment 32

[Ricky Chien [:rickychien] (inactive)]

Attachment: Gaia PR

OK, I tried Yuren's suggestion and patch httpd.js to make test agent running all tests both in apps and dev_apps.

I think it works fine for me. Yuren, I need your help to review this patch:)

Comment 33

[Ricky Chien [:rickychien] (inactive)]

@Yuren,

Sorry this patch has not completed yet.

I'm going to re-assign a review flag to you after finishing incomplete part.

Comment 34

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8423776 [details] [review]
Gaia PR

PR was updated.

Comment 36

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

Comment on attachment 8423776 [details] [review]
Gaia PR

I prefer using APPPATH := apps dev_apps and list all directories in those two directories in javascript, but for now we need to use in makefile rule test-agent-config -- let's fix it in another follow up bug (bug 1014441).

r=yurenju but please fix the issue which is mentioned on github.

Comment 37

[Ricky Chien [:rickychien] (inactive)]

Thanks yuren.

Merged.

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

Comment 38

[Ed Morley [:emorley]]

Reverted for breaking builds:
https://tbpl.mozilla.org/?tree=B2g-Inbound&rev=5dadf0535640

https://github.com/mozilla-b2g/gaia/commit/41e1c20d4f28cfccb6bfba72b69f9a06ef83d866

Did this pass on try?

Comment 39

[Ricky Chien [:rickychien] (inactive)]

Fixed by changing $(PWD) to $(GAIA_DIR).

Waiting for Travis and Try to pass.

https://travis-ci.org/mozilla-b2g/gaia/builds/25841211
https://tbpl.mozilla.org/?tree=Gaia-Try&rev=67bf06ac0b85

Comment 40

[Ricky Chien [:rickychien] (inactive)]

try server with build tests

https://tbpl.mozilla.org/?tree=Try&rev=668924a318cf

Comment 41

[Ricky Chien [:rickychien] (inactive)]

We can see the result of build tests on try[1] had passed. However, it shows two Gu fails but I think it's irrelevant because Gu passed on our Gaia Try[2]

[1] https://tbpl.mozilla.org/?tree=Try&rev=668924a318cf
[2] https://tbpl.mozilla.org/?tree=Gaia-Try&rev=67bf06ac0b85

Comment 42

[Ricky Chien [:rickychien] (inactive)]

Attachment: Gaia PR

Comment 43

[Ricky Chien [:rickychien] (inactive)]

After discussing with yurenju, we think it's safe to land this patch since it passed on try.

Merge.

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

Comment 44

[Carsten Book [:Tomcat]]

reverted since this may have caused https://tbpl.mozilla.org/php/getParsedLog.php?id=40509323&tree=B2g-Inbound bustages

Comment 45

[Ricky Chien [:rickychien] (inactive)]

It's a windows path problem in gaia Makefile. The fail has been fixed and B2G Desktop Windows on TBPL had passed.

TBPL try:
https://tbpl.mozilla.org/?tree=Try&rev=bb3f65ba5bfe

Comment 46

[Ricky Chien [:rickychien] (inactive)]

Attachment: Gaia PR

Comment 47

[Ricky Chien [:rickychien] (inactive)]

Merged.

https://github.com/mozilla-b2g/gaia/commit/8d5b09f43f61f1f08d7195a8ac7378d6c6cb3762