runxpcshelltests.py currently ignores invalid head and tail files: return [os.path.join(test['here'], f).strip() for f in sorted(test['head'].split(' ')) if os.path.isfile(os.path.join(test['here'], f))] I think it would be more appropriate to fatally abort the test if the test manifest is specified incorrectly. Does anyone have a counter-argument?

Why are we calling sorted()? The head and tail files are defined as space delimited list. I would expect the test runner to preserve that order. There are legitimate use cases where one head file needs to be included before another. Forcing a naming convention when the definition format is ordered is just silly.

This patch rejects invalid files by throwing. Files are also defined in the order they were in the source file, not by using sorted(). While I was there, I rewrote the logic. I also used 4 space style, unlike the rest of the file. Try at https://tbpl.mozilla.org/?tree=Try&rev=111dfe724c17

Hmmm... TEST-INFO | /home/cltbld/talos-slave/test/build/xpcshell/tests/services/crypto/component/tests/unit/test_jpake.js | running test ... TEST-PASS | /home/cltbld/talos-slave/test/build/xpcshell/tests/services/crypto/component/tests/unit/test_jpake.js | test passed (time: 606.502ms) TEST-INFO | skipping /home/cltbld/talos-slave/test/build/xpcshell/tests/toolkit/crashreporter/test/unit/test_crash_purevirtual.js | skip-if: os == "linux" || !crashreporter TEST-INFO | skipping /home/cltbld/talos-slave/test/build/xpcshell/tests/toolkit/crashreporter/test/unit/test_crash_runtimeabort.js | skip-if: os == "linux" || !crashreporter TEST-INFO | skipping /home/cltbld/talos-slave/test/build/xpcshell/tests/toolkit/crashreporter/test/unit/test_crash_oom.js | skip-if: os == "linux" || !crashreporter TEST-INFO | skipping /home/cltbld/talos-slave/test/build/xpcshell/tests/toolkit/crashreporter/test/unit/test_crashreporter.js | skip-if: os == "linux" || !crashreporter TEST-INFO | skipping /home/cltbld/talos-slave/test/build/xpcshell/tests/toolkit/crashreporter/test/unit/test_crashreporter_crash.js | skip-if: os == "linux" || !crashreporter TEST-INFO | skipping /home/cltbld/talos-slave/test/build/xpcshell/tests/toolkit/crashreporter/test/unit/test_crashreporter_crash_profile_lock.js | skip-if: os == "linux" || !crashreporter TEST-INFO | skipping /home/cltbld/talos-slave/test/build/xpcshell/tests/toolkit/crashreporter/test/unit/test_crashreporter_appmem.js | skip-if: os == "linux" || !crashreporter Traceback (most recent call last): File "xpcshell/runxpcshelltests.py", line 858, in <module> main() File "xpcshell/runxpcshelltests.py", line 854, in main if not xpcsh.runTests(args[0], testdirs=args[1:], **options.__dict__): File "xpcshell/runxpcshelltests.py", line 645, in runTests testHeadFiles, testTailFiles = self.getHeadAndTailFiles(test) File "xpcshell/runxpcshelltests.py", line 278, in getHeadAndTailFiles return (list(sanitize_list(test['head'])), File "xpcshell/runxpcshelltests.py", line 271, in sanitize_list raise Exception('Included file does not exists: %s' % path) Exception: Included file does not exists: /home/cltbld/talos-slave/test/build/xpcshell/tests/toolkit/crashreporter/test/unit_ipc/head_crashreporter.js program finished with exit code 1

Looks like the crashreporter test was loading the missing head file via load(). So, I removed the head file and initiated a new Try: http://tbpl.mozilla.org/?tree=Try&rev=0d659634aac3

Try run for 111dfe724c17 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=111dfe724c17 Results (out of 24 total builds): exception: 5 success: 10 warnings: 7 failure: 2 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/gszorc@mozilla.com-111dfe724c17

Try run for 0d659634aac3 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=0d659634aac3 Results (out of 30 total builds): success: 28 warnings: 2 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/gszorc@mozilla.com-0d659634aac3

I don't remember why this is written like it is, but your suggested changes sound reasonable. I suspect the sorted() is left over from when we simply listed the directory, before we had manifest files.

Comment on attachment 609873 [details] [diff] [review] Reject invalid head and tail files, v1 Review of attachment 609873 [details] [diff] [review]: ----------------------------------------------------------------- ::: testing/xpcshell/runxpcshelltests.py @@ -262,2 @@ > > - On a remote system, this may be overloaded to list files in a remote directory structure. Note that changing these is going to break remotexpcshelltests.py: http://mxr.mozilla.org/mozilla-central/source/testing/xpcshell/remotexpcshelltests.py#182 It would be nice to fix that even if you can't test it. (You can probably get jmaher or someone to test a patch for you.) @@ +268,4 @@ > > + path = os.path.normpath(os.path.join(test['here'], f)) > + if not os.path.exists(path): > + raise Exception('Included file does not exists: %s' % path) "does not exist" @@ +272,3 @@ > > + if not os.path.isfile(path): > + raise Exception('Included file is not a file: %s' % path) These should both probably mention head/tail, although maybe it'd be obvious from the filename?

Also a test in selftests.py would be nice. :)

Patched remote test runner and added tests. New try at http://tbpl.mozilla.org/?tree=Try&rev=7bd2d3202f26

Comment on attachment 610236 [details] [diff] [review] Reject invalid head and tail files, v2 Review of attachment 610236 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for writing tests and fixing the remote version! ::: testing/xpcshell/selftest.py @@ +225,5 @@ > + # The actual return value is never checked because we raise. > + self.assertTestResult(True) > + except Exception, ex: > + raised = True > + self.assertEquals(ex.message[0:9], "head file") There's a TestCase.assertRaises that you can use here: http://docs.python.org/library/unittest.html#unittest.TestCase.assertRaises This leads me to a follow-up I hadn't thought of. Should the harness catch these, log something and return False? Doesn't matter too much in practice, I'll leave that up to you.

(In reply to Ted Mielczarek [:ted] from comment #11) > There's a TestCase.assertRaises that you can use here: > http://docs.python.org/library/unittest.html#unittest.TestCase.assertRaises I /thought/ this helper wasn't in Python 2.5, so I didn't use it. But since you mentioned it, I looked it up and it does exist in Python 2.5. Unfortunately you can't use it as a context manager until Python 2.7, so it isn't as clean. As a result, I think I'll hold off adopting it. > This leads me to a follow-up I hadn't thought of. Should the harness catch > these, log something and return False? Doesn't matter too much in practice, > I'll leave that up to you. In my mind, a missing head or tail file is synonymous with a malformed manifest file: the manifest isn't proper. And, AFAICT manifestparser.py raises when errors are encountered. So, I think I'm being consistent. The exception will propagate and result in a non-0 exit code. Today, exceptions are only trapped inside the process execution bit of the test loop. i.e. the individual test set-up code (of which head and tail extraction is part) is untrapped. So, again, I think raising is consistent with existing behavior. We could certainly change things so more of the inner test logic is trapped. But, that would be for another bug, IMO.

Try seemed happy, so I landed. https://hg.mozilla.org/integration/mozilla-inbound/rev/e28a79957b4a

Try run for 7bd2d3202f26 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=7bd2d3202f26 Results (out of 136 total builds): exception: 3 success: 127 warnings: 6 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/gszorc@mozilla.com-7bd2d3202f26