Julien Wajsberg [:julienw] Reporter
	Description • 11 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.

	Etienne Segonzac (:etienne)
	Comment 1 • 11 years ago

Not convinced we want to add this much overhead to the test-agent and encourage badly written tests :s

	(inactive after 6/18) Alive Kuo [:alive]
	Comment 2 • 11 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

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

	Julien Wajsberg [:julienw] Reporter
	Comment 3 • 11 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... :(

	James Lal [:lightsofapollo]
	Comment 4 • 11 years ago

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

	Greg Weng [:snowmantw][:gweng][:λ]
	Comment 5 • 11 years ago

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.

	Julien Wajsberg [:julienw] Reporter
	Comment 6 • 11 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.

	Julien Wajsberg [:julienw] Reporter
	Comment 8 • 10 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

	Kevin Grandon :kgrandon Assignee
	Comment 9 • 10 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.

	Kevin Grandon :kgrandon Assignee
	Comment 10 • 10 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.

	Julien Wajsberg [:julienw] Reporter
	Comment 11 • 10 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).

	Kevin Grandon :kgrandon Assignee
	Comment 12 • 10 years ago

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.

	Kevin Grandon :kgrandon Assignee
	Comment 13 • 10 years ago

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

	Julien Wajsberg [:julienw] Reporter
	Comment 14 • 10 years ago

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.

	Kevin Grandon :kgrandon Assignee
	Comment 15 • 10 years ago

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

	Julien Wajsberg [:julienw] Reporter
	Comment 16 • 10 years ago

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.

	Kevin Grandon :kgrandon Assignee
	Comment 17 • 10 years ago

Landed: https://github.com/mozilla-b2g/gaia/commit/57c273a5d7fb9da8a4299964a8f54ad5a709436c

	Jared Hirsch [:jhirsch] (he/him) (Needinfo please)
	Comment 18 • 10 years ago

\o/ awesome!

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

	Julien Wajsberg [:julienw] Reporter
	Comment 19 • 10 years ago

This is not the only difference though ;)

	Julien Wajsberg [:julienw] Reporter
	Comment 21 • 10 years ago

Trying to uplift to v1.4: https://github.com/mozilla-b2g/gaia/pull/21286

	Tim Guan-tin Chien [:timdream] (please needinfo)
	Comment 22 • 10 years ago

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 .

	Tim Guan-tin Chien [:timdream] (please needinfo)
	Comment 24 • 10 years ago

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

	Tim Guan-tin Chien [:timdream] (please needinfo)
	Comment 25 • 10 years ago

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?

	Julien Wajsberg [:julienw] Reporter
	Comment 27 • 10 years ago

v1.4: 71aa8a3697e8daacdaee3d447a38ee10f13d5b54