[test-agent] run each test separately in its own sandbox

RESOLVED FIXED

Status

Firefox OS
Gaia::TestAgent
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: julienw, Assigned: kgrandon)

Tracking

unspecified
x86_64
Linux
Dependency tree / graph

Firefox Tracking Flags

(b2g-v1.4 fixed)

Details

Attachments

(2 attachments)

(Reporter)

Description

5 years ago
Right now, when running unit tests, one badly written test may affect subsequent tests. This is less than perfect because quite often in these cases the badly written test is not the one failing, rather it makes an innocent test fail, even if that innocent test runs ok when it runs alone.

As a matter of fact, a test should not have a different behaviour whether it runs alone or along with other tests.

It would be way more reliable to run each test in its own sandbox instead of running them all in the same sandbox.
Not convinced we want to add this much overhead to the test-agent and encourage badly written tests :s
(In reply to Etienne Segonzac (:etienne) from comment #1)
> Not convinced we want to add this much overhead to the test-agent and
> encourage badly written tests :s

Although I don't know how to achieve multi-sandbox, but writing a test would depend on other unit tests disappointed me.
(Reporter)

Comment 3

5 years ago
(In reply to Etienne Segonzac (:etienne) from comment #1)
> Not convinced we want to add this much overhead to the test-agent and
> encourage badly written tests :s

That was also my position but I'm quite fed up of side-effects... :(
This is not particularly difficult but I am really curious _why_ we are having these bad tests? We can teach test-agent to run them in separate sandboxes with a flag enabled per app ( I rarely run into this issue outside of db opening/closing which is fixed in calendar ).

I suspect this relates to bad mocks, global populate and global state manipulation which are all things running single-sandboxed tests would fix but there are other ways to fix those problems as well...
(Reporter)

Updated

5 years ago
Component: Gaia → Gaia::TestAgent
(Reporter)

Updated

5 years ago
Summary: [test-agent] run each test separately → [test-agent] run each test separately in its own sandbox
Yuren and I have discussed this. We found that in our our apps, iframes can be accessed via a 'sandbox' attribute. We thought that if we can put every new tests under such iframe, and run the setup first, we would get a sandbox and there would be no side-effects. Another thing is if these iframe can run parallel, or concurrently, we can reduce the waiting time to wait these tests finished. However, Thinker said our iframes run in the same process with the app, so we can't benefit from this method.
(Reporter)

Comment 6

5 years ago
(In reply to Greg Weng [:snowmantw][:gweng][:λ] from comment #5)
> Yuren and I have discussed this. We found that in our our apps, iframes can
> be accessed via a 'sandbox' attribute. We thought that if we can put every
> new tests under such iframe, and run the setup first, we would get a sandbox
> and there would be no side-effects.

Yeah, I agree, that's what I explained in the other bug.

> Another thing is if these iframe can run
> parallel, or concurrently, we can reduce the waiting time to wait these
> tests finished. However, Thinker said our iframes run in the same process
> with the app, so we can't benefit from this method.

This is for another bug, but I think this is one of the goals that James has for test-agent v2.
Duplicate of this bug: 971634
(Reporter)

Comment 8

4 years ago
To solve this, I'd first try to tickle the code in [1].

If this works, this would be better to change the function name too.

[1] https://github.com/mozilla-b2g/gaia/blob/434acd8400468b8216341d67731b072b24c53820/test_apps/test-agent/common/test/agent.js#L83-L107
(Assignee)

Comment 9

4 years ago
This seems to work locally, but is a bit slow: https://github.com/mozilla-b2g/gaia/pull/17720

Let me try this on travis to see how it does. If this takes dramatically longer, we may have to find another solution as we don't really have the travis capacity to add a much longer job right now.
(Assignee)

Comment 10

4 years ago
Surprisingly, the time it took on travis is comparable to what we do today. To get this working though, we would need to fix all tests in tests/python/gaia-unit-tests/gaia_unit_test/disabled.json to run as standalone tests. I will file a bug to do so if one does not exist.
(Assignee)

Updated

4 years ago
Depends on: 989017
(Reporter)

Comment 11

4 years ago
thanks !

Bug 943163 and bug 973873 should make the full run faster if we use sandboxes. I have WIP patches for both of them, (unposted for bug 943163, I'll do it later if you want to move forward).
(Assignee)

Comment 12

4 years ago
Created attachment 8398045 [details] [review]
Github pull request

The name of the function still needs to be updated, but this mostly works. I'm going to focus on getting the tests to all pass in a sandbox, so if anyone wants to steal this feel free to.
(Assignee)

Comment 13

4 years ago
Comment on attachment 8398045 [details] [review]
Github pull request

Hey - what do you guys think? I managed to get a travis green after fixing a bunch of tests, and I think this could catch a lot of stuff before we go to TBPL.

I have not yet updated the name of the function because that would require us to update the node module, and I'm not sure it's necessary. (Node_modules updates are currently broken from updating due to a sockit-to-me compilation problem).
Attachment #8398045 - Flags: review?(jlal)
Attachment #8398045 - Flags: review?(felash)
(Assignee)

Updated

4 years ago
Blocks: 991446
(Reporter)

Comment 14

4 years ago
Comment on attachment 8398045 [details] [review]
Github pull request

just left a comment on github, I think this will be simpler
otherwise this looks to work, I checked in the dev tools that we have a new iframe url for each time.
(Assignee)

Comment 15

4 years ago
(In reply to Julien Wajsberg [:julienw] from comment #14)
> Comment on attachment 8398045 [details] [review]
> Github pull request
> 
> just left a comment on github, I think this will be simpler
> otherwise this looks to work, I checked in the dev tools that we have a new
> iframe url for each time.

Thanks. I updated the pull request.

I would prefer to keep concatenating the env variable as host + url as the url may be something generic like: test/unit/dialog_test.js. I would not want to run the chance of having two files be in the same sandbox, so it seems safer to concatenate host + url for now.
(Reporter)

Comment 16

4 years ago
Comment on attachment 8398045 [details] [review]
Github pull request

r=me

please double look if just using the parameter "test" as environment doesn't work, I think it does.
Attachment #8398045 - Flags: review?(felash) → review+
(Assignee)

Comment 17

4 years ago
Landed: https://github.com/mozilla-b2g/gaia/commit/57c273a5d7fb9da8a4299964a8f54ad5a709436c
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
\o/ awesome!

I have hit the Travis/TBPL problem repeatedly, this is going to be a huge time saver :-)
(Reporter)

Comment 19

4 years ago
This is not the only difference though ;)
(Reporter)

Updated

4 years ago
Duplicate of this bug: 964117
(Assignee)

Updated

4 years ago
Attachment #8398045 - Flags: review?(jlal)
(Reporter)

Comment 21

4 years ago
Trying to uplift to v1.4: https://github.com/mozilla-b2g/gaia/pull/21286
Assignee: nobody → kgrandon
I re-run https://travis-ci.org/mozilla-b2g/gaia/jobs/28974093 because it was errored with

Waiting for server on port 8080 failed.

The command "./node_modules/.bin/travisaction $CI_ACTION before_script" failed and exited with 1 during .
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #22)
> I re-run https://travis-ci.org/mozilla-b2g/gaia/jobs/28974093 because it was
> errored with
> 
> Waiting for server on port 8080 failed.
> 
> The command "./node_modules/.bin/travisaction $CI_ACTION before_script"
> failed and exited with 1 during .

I've found two test errors and I fixed it accordingly. Also bug 1033408 is gone \o/.
Comment on attachment 8449994 [details] [review]
mozilla-b2g:v1.4 PR#21324

Julien, this should work on v1.4 and it fixes the perma-red. Should I land it?
Attachment #8449994 - Flags: feedback?(felash)
Duplicate of this bug: 1033408
(Reporter)

Comment 27

4 years ago
v1.4: 71aa8a3697e8daacdaee3d447a38ee10f13d5b54
status-b2g-v1.4: --- → fixed
You need to log in before you can comment on or make changes to this bug.