888526 - use the profile dir instead of the bin dir for contentpref xpcshell tests

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Comment 1

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: use do_get_profile

Comment 2

[(no longer active)]

Comment on attachment 769253 [details] [diff] [review]
use do_get_profile

Review of attachment 769253 [details] [diff] [review]:
-----------------------------------------------------------------

Hmm, I'm not sure if I'm the best reviewer for this patch.  The bug doesn't really describe what the problem is and how this patch tries to fix it, which doesn't help much either...

Comment 3

[Mihnea Dobrescu-Balaur (:mihneadb)]

This is needed so we can run xpcshell tests concurrently. Currently this is not possible for the contentpref tests because they use the bin directory which is shared. This patch makes them use the profile dir which is individual to every test that is running.

Comment 4

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: use the profile dir

Comment 5

[Mihnea Dobrescu-Balaur (:mihneadb)]

try run: https://tbpl.mozilla.org/?tree=Try&rev=b9ab7fd28b73

Comment 6

[(no longer active)]

Why are you limiting this to runningInParent then?

Comment 7

[Mihnea Dobrescu-Balaur (:mihneadb)]

I can only call do_get_profile when runningInParent and there is only one ipc test (so no concurrency issues atm).
I could change the function so that when not runningInParent it uses the XPCSHELL_TEST_PROFILE_DIR env variable. Is that ok?

Comment 8

[(no longer active)]

(In reply to comment #7)
> I can only call do_get_profile when runningInParent and there is only one ipc
> test (so no concurrency issues atm).
> I could change the function so that when not runningInParent it uses the
> XPCSHELL_TEST_PROFILE_DIR env variable. Is that ok?

I think we should just use do_get_profile() unconditionally.

Comment 9

[Mihnea Dobrescu-Balaur (:mihneadb)]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #8)
> (In reply to comment #7)
> > I can only call do_get_profile when runningInParent and there is only one ipc
> > test (so no concurrency issues atm).
> > I could change the function so that when not runningInParent it uses the
> > XPCSHELL_TEST_PROFILE_DIR env variable. Is that ok?
> 
> I think we should just use do_get_profile() unconditionally.

Well as I said, that is not possible :).

http://mxr.mozilla.org/mozilla-central/source/testing/xpcshell/head.js#927

Comment 10

[(not currently active) Ted Mielczarek]

Comment on attachment 772821 [details] [diff] [review]
use the profile dir

Review of attachment 772821 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/contentprefs/tests/unit/head_contentPrefs.js
@@ +85,5 @@
>     */
>    getProfileDir: function ContentPrefTest_getProfileDir() {
> +    if (runningInParent) {
> +      return do_get_profile();
> +    }

It seems like this is not going to do the right thing if it's running in a child process. Does this code get run in a child process? If so, you should fix it to work properly there. If not, then you can probably do a lot more code removal in this test.

::: toolkit/components/contentprefs/tests/unit_ipc/xpcshell.ini
@@ +2,5 @@
>  head = head_contentPrefs.js
>  tail = tail_contentPrefs.js
>  
> +# it these tests run in parallel you need to make getProfileDirectory
> +# return the correct profile in the _child_ process

I don't think putting a comment here is sufficient. We should fix the test.

Comment 11

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: use the profile dir instead of the bin dir for contentpref xpcshell tests,

Does this make sense?

Comment 12

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: use the profile dir instead of the bin dir for contentpref xpcshell tests,

Forgot to revert the manifest file.

Comment 13

[Mihnea Dobrescu-Balaur (:mihneadb)]

try run: https://tbpl.mozilla.org/?tree=Try&rev=1d95bf4a7a89

Comment 14

[(not currently active) Ted Mielczarek]

Comment on attachment 774286 [details] [diff] [review]
use the profile dir instead of the bin dir for contentpref xpcshell tests,

Review of attachment 774286 [details] [diff] [review]:
-----------------------------------------------------------------

If this works then it's fine. I'm sure there's more code that could be removed, but it's not terribly important.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/90b078a81eef

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/90b078a81eef