Closed Bug 864178 Opened 12 years ago Closed 11 years ago

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

Categories

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

x86_64
Linux
defect
Not set
normal

Tracking

(b2g-v1.4 fixed)

RESOLVED FIXED
Tracking Status
b2g-v1.4 --- fixed

People

(Reporter: julienw, Assigned: kgrandon)

References

Details

Attachments

(2 files)

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.
(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...
Component: Gaia → Gaia::TestAgent
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.
(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.
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
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.
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.
Depends on: 989017
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).
Attached file Github pull request (deleted) —
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.
Comment on attachment 8398045 [details] 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)
Blocks: 991446
Comment on attachment 8398045 [details] 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.
(In reply to Julien Wajsberg [:julienw] from comment #14) > Comment on attachment 8398045 [details] > 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.
Comment on attachment 8398045 [details] 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+
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
\o/ awesome! I have hit the Travis/TBPL problem repeatedly, this is going to be a huge time saver :-)
This is not the only difference though ;)
Attachment #8398045 - Flags: review?(jlal)
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] 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)
v1.4: 71aa8a3697e8daacdaee3d447a38ee10f13d5b54
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: