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

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.

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.

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

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

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

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?

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.

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

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

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

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.

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

Refactored the try-delete logic in a function.

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

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 on attachment 774927 [details] [diff] [review] simple test file keeping r+

Fixed the two problems.

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

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

s/print_stdout/self.print_stdout

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

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

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

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

Calling setuptempdir() in buildenv now.

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

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

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

Making sure the directory exists now.

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

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

Comment on attachment 776470 [details] [diff] [review] squashed patch squashing r+'s from :ted and :gbrown