892021 - add a do_get_tempdir function to head.js

Status: RESOLVED FIXED

(Testing :: XPCShell Harness, defect)

Description

[Mihnea Dobrescu-Balaur (:mihneadb)]

Currently when an xpcshell test wants to use a temp dir, it uses the system global one (i.e. /tmp on UNIX). This is not desired when running the tests concurrently.

There should be a do_get_tempdir function in head.js that returns the path of the profile dir (since every test gets one), without actually setting up the profile.

Comment 1

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: add a do_get_tempdir function to head.js

Comment 2

[Gregory Szorc [:gps]]

Comment on attachment 773459 [details] [diff] [review]
add a do_get_tempdir function to head.js

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

There are two use cases for retrieving a temp directory: 1) get the global temporary directory (as implemented) 2) get a new, empty temp directory on every call.

I think #2 is more useful and what tests would make better use of. Can we have the function maintain a counter and create and return a new directory on every invocation?

Comment 3

[(not currently active) Ted Mielczarek]

Note that this is not strictly global, this is the temp directory that the Python harness creates. This will be unique per-invocation of xpcshell. I don't think there's much value in making additional temp directories on top of that. Tests can create subdirs if need be.

Comment 4

[Marco Bonardo [:mak]]

why would you want a different temp dir at each invokation, most of the services won't need such thing, they will just use TmpD, that is one, and if they need a new temp folder they can just create new ones inside the dir they get from this method.

Comment 5

[Mihnea Dobrescu-Balaur (:mihneadb)]

(In reply to Gregory Szorc [:gps] from comment #2)
> Comment on attachment 773459 [details] [diff] [review]
> add a do_get_tempdir function to head.js
> 
> Review of attachment 773459 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> There are two use cases for retrieving a temp directory: 1) get the global
> temporary directory (as implemented) 2) get a new, empty temp directory on
> every call.
> 
> I think #2 is more useful and what tests would make better use of. Can we
> have the function maintain a counter and create and return a new directory
> on every invocation?


The problem that I see with #2 is that you might want to use that temp dir in multiple places - like set something up in the head file and then use that temp dir in a test. Sure, you could use a global var but from what I saw while fixing tests a global might not be enough.

Comment 6

[Gregory Szorc [:gps]]

You've all convinced me. Can we at least get a fresh temporary directory instead of sharing it with the profile root directory?

Comment 7

[Marco Bonardo [:mak]]

I actually suggested to create a uniquely named folder in TmpD, not to reuse the profile dir.

Comment 8

[(not currently active) Ted Mielczarek]

Alternate suggestion: we create /{profile,tmp} subdirs under the directory that Python creates, and hand one out from do_get_tmp and the other for do_get_profile?

Comment 9

[Marco Bonardo [:mak]]

whatever, I just thought was easy to use nsIFile.createUnique with a recognizable name like xpcshell-temp-dir or such. But I suppose having it in the python dir may end up into a faster cleanup of the trash put into it.

Comment 10

[Gregory Szorc [:gps]]

(In reply to Marco Bonardo [:mak] from comment #7)
> I actually suggested to create a uniquely named folder in TmpD, not to reuse
> the profile dir.

We want things under the temp directory created by Python so it will always be cleaned up. Otherwise, tests could modify the filesystem outside of the "sandbox" and those changes would linger forever.

Of course, tests could bypass this protection today if they wanted. We'd ideally execute things inside chroots or something. But that's for another bug.

Comment 11

[Marco Bonardo [:mak]]

it's a very good reason, go for it!

Comment 12

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: create the tempdir in the harness and use it from head.js

I'm not sure if I should handle cleaning the tempdir differently.

Comment 13

[(not currently active) Ted Mielczarek]

Comment on attachment 773557 [details] [diff] [review]
create the tempdir in the harness and use it from head.js

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

::: testing/xpcshell/head.js
@@ +931,5 @@
> +  let file = Components.classes["@mozilla.org/file/local;1"]
> +                       .createInstance(Components.interfaces.nsILocalFile);
> +  file.initWithPath(profd);
> +  return file;
> +}

Can you add a test for this somewhere in testing/xpcshell/example/unit? It can be simple, just do_get_tempdir and assert that it exists and that you can write a file under it.

::: testing/xpcshell/runxpcshelltests.py
@@ +316,5 @@
> +        tempDir = mkdtemp()
> +        self.env["XPCSHELL_TEST_TEMP_DIR"] = tempDir
> +        if self.interactive:
> +            self.log.info("TEST-INFO | temp dir is %s" % tempDir)
> +        return tempDir

This is going to need an equivalent change in remotexpcshelltests.py so as not to break Android tests. You should be able to mostly just copy the setupProfileDir code there.

@@ +1046,5 @@
>  
> +            try:
> +                self.removeDir(self.tempDir)
> +            except Exception:
> +                self.log.warning("TEST-INFO | %s | Failed to clean up the test temp directory (%s)." % (name, self.profileDir))

It might make sense to refactor the "try again if removing the directory fails" logic from above into a function so we can use it for both the profile dir and the temp dir, but we can punt that to the future if it becomes a problem.

Comment 14

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: simple test file

Comment 15

[Mihnea Dobrescu-Balaur (:mihneadb)]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #13)
> Comment on attachment 773557 [details] [diff] [review]
> create the tempdir in the harness and use it from head.js
> 
> ::: testing/xpcshell/runxpcshelltests.py
> @@ +316,5 @@
> > +        tempDir = mkdtemp()
> > +        self.env["XPCSHELL_TEST_TEMP_DIR"] = tempDir
> > +        if self.interactive:
> > +            self.log.info("TEST-INFO | temp dir is %s" % tempDir)
> > +        return tempDir
> 
> This is going to need an equivalent change in remotexpcshelltests.py so as
> not to break Android tests. You should be able to mostly just copy the
> setupProfileDir code there.

This is not trivial because we don't have an equivalent "mkdtemp" in devicemanager. We only have getTempDir which returns the equivalent of "/tmp" on the device.

A "good enough" (for now!) solution would be to assume that there's only one connection at a time to the device and use mkdtemp locally (on the PC) and push that dir (dir name) to the device's getTempDir() directory.

This will not work properly when multiple PCs will control the same devices, but for just one it should work just fine.


What do you think?

Comment 16

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: tempdir

Refactored the try-delete logic in a function.

Comment 17

[Geoff Brown [:gbrown]]

remotexpcshelltests.py already has a notion of a simple temp dir: see references to TMPDIR and remoteTmpDir. 

I would be tempted to simply override setupTempDir in remotexpcshelltests.py:

    def setupTempDir(self):
        self.env["XPCSHELL_TEST_TEMP_DIR"] = self.remoteTmpDir
        return self.remoteTmpDir

That does not address any concurrency concerns of course. If we want/need multiple unique temp dirs on the device, I suspect there's no easy to do that.

Comment 18

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: update remote harness

Comment 19

[(not currently active) Ted Mielczarek]

Comment on attachment 774162 [details] [diff] [review]
simple test file

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

Thanks!

::: testing/xpcshell/example/unit/test_do_get_tempdir.js
@@ +1,5 @@
> +/* -*- Mode: Java; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* vim:set ts=2 sw=2 sts=2 et: */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

Note that you can license test files as public domain if you so choose (I usually do):
http://www.mozilla.org/MPL/headers/

@@ +14,5 @@
> +  tmpd.append("testfile");
> +  tmpd.create(Ci.nsIFile.NORMAL_FILE_TYPE, 600);
> +  do_check_true(tmpd.exists());
> +  tmpd.remove(false);
> +  do_check_false(tmpd.exists());

Remove these last two lines, the Python harness cleans up the temp directory, so there's no need for this test.

Comment 20

[(not currently active) Ted Mielczarek]

Comment on attachment 774263 [details] [diff] [review]
tempdir

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

::: testing/xpcshell/head.js
@@ +918,5 @@
>  }
>  
>  /**
> + * Returns the directory for a temp dir, which uses the path
> + * of the profile dir, since every test gets its own.

This comment is no longer correct, since you're creating a different temp dir.

@@ +926,5 @@
> +function do_get_tempdir() {
> +  let env = Components.classes["@mozilla.org/process/environment;1"]
> +                      .getService(Components.interfaces.nsIEnvironment);
> +  // the python harness sets this in the environment for us
> +  let profd = env.get("XPCSHELL_TEST_TEMP_DIR");

It's a little odd to call this variable "profd" in this function.

Comment 21

[(not currently active) Ted Mielczarek]

It seems fine to not deal with concurrency in the remote harness for now, as long as we document that and enforce it in the code. If we change our minds in the future we can sort it out then.

Comment 22

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: simple test file

Comment 23

[Mihnea Dobrescu-Balaur (:mihneadb)]

Comment on attachment 774927 [details] [diff] [review]
simple test file

keeping r+

Comment 24

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: tempdir

Fixed the two problems.

Comment 25

[Geoff Brown [:gbrown]]

Comment on attachment 774940 [details] [diff] [review]
tempdir

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

::: testing/xpcshell/runxpcshelltests.py
@@ +853,5 @@
> +            except Exception:
> +                message = "TEST-UNEXPECTED-FAIL | %s | Failed to clean up directory: %s" % (name, sys.exc_info()[1])
> +                self.log.error(message)
> +                print_stdout(stdout)
> +                print_stdout(traceback.format_exc())

This fails for me. I think you need self.print_stdout(...)

Comment 26

[Mihnea Dobrescu-Balaur (:mihneadb)]

(In reply to Geoff Brown [:gbrown] from comment #25)
> Comment on attachment 774940 [details] [diff] [review]
> tempdir
> 
> Review of attachment 774940 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: testing/xpcshell/runxpcshelltests.py
> @@ +853,5 @@
> > +            except Exception:
> > +                message = "TEST-UNEXPECTED-FAIL | %s | Failed to clean up directory: %s" % (name, sys.exc_info()[1])
> > +                self.log.error(message)
> > +                print_stdout(stdout)
> > +                print_stdout(traceback.format_exc())
> 
> This fails for me. I think you need self.print_stdout(...)

Whoops, absolutely! I didn't encounter a failing test while.. testing.

Comment 27

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: tempdir

s/print_stdout/self.print_stdout

Comment 28

[Mihnea Dobrescu-Balaur (:mihneadb)]

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

Comment 29

[Mihnea Dobrescu-Balaur (:mihneadb)]

(In reply to Mihnea Dobrescu-Balaur (:mihneadb) from comment #28)
> try run: https://tbpl.mozilla.org/?tree=Try&rev=95bebff7fc58

I'm confused about the failure on Android. According to the docs on initWithFilePath, NS_ERROR_FILE_UNRECOGNIZED_PATH means that the given path is not absolute.

Could it be that the tmpDirPath is too long to fit in the env var or something like that? Although, this does not explain why it only happened once.

Comment 30

[Mihnea Dobrescu-Balaur (:mihneadb)]

Via some debug print statements I found that after || let path = env.get("XPCSHELL_TEST_TEMP_DIR"); ||, path is the empty string. (Or undefined)

Comment 31

[Geoff Brown [:gbrown]]

At issue is the timing of setting the env. The remote xpcshell picks up its environment from the "wrapper script", which is pushed once at setup time. This patch modifies the env after the wrapper script has been created and pushed to the device.

Simple fix: call setupTempDir from buildEnvironment, before the call to pushWrapper. (This can be in addition to the existing call of setupTempDir in runTest, if you wish.)

Comment 32

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: update the remote harness

Calling setuptempdir() in buildenv now.

Comment 33

[Mihnea Dobrescu-Balaur (:mihneadb)]

https://tbpl.mozilla.org/?tree=Try&rev=ad1bbcc4b0b1
^ new try run.

Comment 34

[Geoff Brown [:gbrown]]

This *looks* right but keeps failing. Is the per-test cleanup deleting the remote tmp dir? (I think the remote tmp dir is only created once.)

Comment 35

[Mihnea Dobrescu-Balaur (:mihneadb)]

Yes, it does. Not sure how to address this.

Comment 36

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: temp dir in remote harness

Making sure the directory exists now.

Comment 37

[Mihnea Dobrescu-Balaur (:mihneadb)]

Finally try is open, here's the new run - https://tbpl.mozilla.org/?tree=Try&rev=2c1e35691626

Comment 38

[(not currently active) Ted Mielczarek]

Can you fold all three of these patches together for landing? They should really land as one changeset.

Comment 39

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: squashed patch

Comment 40

[Mihnea Dobrescu-Balaur (:mihneadb)]

Comment on attachment 776470 [details] [diff] [review]
squashed patch

squashing r+'s from :ted and :gbrown

Comment 41

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d090154d58b9

Comment 42

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d090154d58b9