739753 - xpcshell test runner silently drops invalid head and tail files

Status: RESOLVED FIXED

(Testing :: XPCShell Harness, defect)

Description

[Gregory Szorc [:gps]]

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?

Comment 1

[Gregory Szorc [:gps]]

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.

Comment 2

[Gregory Szorc [:gps]]

Attachment: Reject invalid head and tail files, v1

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

Comment 3

[Gregory Szorc [:gps]]

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

Comment 4

[Gregory Szorc [:gps]]

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

Comment 5

[Mozilla RelEng Bot]

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/[e-mail]-111dfe724c17

Comment 6

[Mozilla RelEng Bot]

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/[e-mail]-0d659634aac3

Comment 7

[(not currently active) Ted Mielczarek]

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 8

[(not currently active) Ted Mielczarek]

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?

Comment 9

[(not currently active) Ted Mielczarek]

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

Comment 10

[Gregory Szorc [:gps]]

Attachment: Reject invalid head and tail files, v2

Patched remote test runner and added tests.

New try at http://tbpl.mozilla.org/?tree=Try&rev=7bd2d3202f26

Comment 11

[(not currently active) 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.

Comment 12

[Gregory Szorc [:gps]]

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

Comment 13

[Gregory Szorc [:gps]]

Try seemed happy, so I landed.

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

Comment 14

[Mozilla RelEng Bot]

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/[e-mail]-7bd2d3202f26

Comment 15

[Matt Brubeck (:mbrubeck)]

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