Closed Bug 739753 Opened 10 years ago Closed 10 years ago

xpcshell test runner silently drops invalid head and tail files

Categories

(Testing :: XPCShell Harness, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla14

People

(Reporter: gps, Assigned: gps)

References

Details

(Whiteboard: [qa-])

Attachments

(1 file, 1 obsolete file)

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
Assignee: nobody → gps
Status: NEW → ASSIGNED
Attachment #609873 - Flags: review?(ted.mielczarek)
Blocks: 731494
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?
Attachment #609873 - Flags: review?(ted.mielczarek) → review+
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
Attachment #609873 - Attachment is obsolete: true
Attachment #610236 - Flags: review?(ted.mielczarek)
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.
Attachment #610236 - Flags: review?(ted.mielczarek) → review+
(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
Whiteboard: [inbound]
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
https://hg.mozilla.org/mozilla-central/rev/e28a79957b4a
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla14
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.