498128 - xpcshell-tests: add option to enable/disable file logging

Status: RESOLVED FIXED

(Testing :: XPCShell Harness, enhancement)

Description

[Serge Gautherie (:sgautherie)]

(In reply to bug 484298 comment 3)
> Does it slow the tests down a lot? Do they take up a lot of disk space?

Actually, it does, especially (now) after bug 362433.

(In reply to bug 484298 comment 4)
> I don't think we need a special argument, we should just always write the log
> or never write the log.

I'd like to either:
*Not log by default and add an option to enable logging (for developers).
*Log by default and add an option to disable logging (for test boxes and testers).

Ted, what do you think?

Comment 1

[Serge Gautherie (:sgautherie)]

Ted, ping for answer.

Comment 2

[(not currently active) Ted Mielczarek]

I don't know. I just want it to "do the right thing" and make people not think about it. I guess that means logging by default. Maybe we can make the top-level xpcshell-tests target disable logging, but the per-directory ones enable it?

Comment 3

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) Support a --no-logfiles option

(In reply to comment #2)
> Maybe we can make the top-level xpcshell-tests target disable logging,
> but the per-directory ones enable it?

I like that! ;-)

Comment 4

[Serge Gautherie (:sgautherie)]

Attachment: (Av2) Support --no-logfiles in xpcst suite, improve dir/file handling in 3 suites

Av1, with dir/file handling improvement too.

Comment 5

[Serge Gautherie (:sgautherie)]

Comment on attachment 399310 [details] [diff] [review]
(Av2) Support --no-logfiles in xpcst suite, improve dir/file handling in 3 suites


Succeeded as try-2a5369c1706f.

Comment 6

[(not currently active) Ted Mielczarek]

Comment on attachment 399310 [details] [diff] [review]
(Av2) Support --no-logfiles in xpcst suite, improve dir/file handling in 3 suites

-  # Start with a clean slate.
-  shutil.rmtree(profileDir, True)
-  os.mkdir(profileDir)
-  # reftest should only need the dump pref set
+  # Set preferences.

What's the point of these changes?

-def runTests(xpcshell, testdirs=[], xrePath=None, testPath=None,
-             manifest=None, interactive=False, symbolsPath=None):
-  """Run the tests in |testdirs| using the |xpcshell| executable.
+def runTests(xpcshell, xrePath, symbolsPath,
+             manifest, testdirs, testPath,
+             interactive, logfiles):

Don't change the default values here.

+        if profileDir:
+          # shutil.rmtree(profileDir, True)
+          shutil.rmtree(profileDir)

I don't think it matters whether we pass "ignoreErrors=True" or not, but get rid of the commented version.

-  if not runTests(args[0], testdirs=args[1:],
-                  xrePath=options.xrePath,
-                  testPath=options.testPath,
-                  interactive=options.interactive,
-                  manifest=options.manifest,
-                  symbolsPath=options.symbolsPath):
+  if not runTests(args[0], options.xrePath, options.symbolsPath,
+                  options.manifest, args[1:], options.testPath,
+                  options.interactive, options.logfiles):

Don't change this. I like the keyword argument style.

Comment 7

[Serge Gautherie (:sgautherie)]

Attachment: (Av3) Support --no-logfiles in xpcst suite, improve dir/file handling in 3 suites [Checkin: See comment 9 & 10 & 11]

Av2, with comment 6 suggestion(s).

(In reply to comment #6)
> (From update of attachment 399310 [details] [diff] [review])
> -  # Start with a clean slate.
> -  shutil.rmtree(profileDir, True)
> -  os.mkdir(profileDir)
> -  # reftest should only need the dump pref set
> +  # Set preferences.
> 
> What's the point of these changes?

Calling code is
{
    profileDir = mkdtemp()
    createReftestProfile(options, profileDir)
}

mkdtemp() is supposed to return an already clean dir,
so it seems rmtree+mkdir is useless and asking for trouble.
(And it helps keep all "same" code the same...)
If you do want to keep them, I would suggest to add a comment to explain why they're needed.

(And the code is now setting up 2 prefs, not just 1.)

Comment 8

[(not currently active) Ted Mielczarek]

Comment on attachment 401333 [details] [diff] [review]
(Av3) Support --no-logfiles in xpcst suite, improve dir/file handling in 3 suites
[Checkin: See comment 9 & 10 & 11]

                   interactive=options.interactive,
-                  manifest=options.manifest,
-                  symbolsPath=options.symbolsPath):
+                  options.logfiles):

Pass logfiles as a keyword argument,
logfiles=options.logfiles

r=me with that change.

Thanks for the explanation, I guess either that code has always been stupid, or it became stupid during a refactoring.

Comment 9

[Serge Gautherie (:sgautherie)]

Comment on attachment 401333 [details] [diff] [review]
(Av3) Support --no-logfiles in xpcst suite, improve dir/file handling in 3 suites
[Checkin: See comment 9 & 10 & 11]


http://hg.mozilla.org/mozilla-central/rev/72ae5f35e8fa
Av3, with comment 8 suggestion(s).

Comment 10

[Serge Gautherie (:sgautherie)]

Comment on attachment 401333 [details] [diff] [review]
(Av3) Support --no-logfiles in xpcst suite, improve dir/file handling in 3 suites
[Checkin: See comment 9 & 10 & 11]


http://hg.mozilla.org/releases/mozilla-1.9.2/rev/1c148973c1fb

Comment 11

[Serge Gautherie (:sgautherie)]

Comment on attachment 401333 [details] [diff] [review]
(Av3) Support --no-logfiles in xpcst suite, improve dir/file handling in 3 suites
[Checkin: See comment 9 & 10 & 11]


http://hg.mozilla.org/releases/mozilla-1.9.1/rev/6b19fc023243