857966 - Tests referenced by manifest files that do not exist are silently ignored

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[Zac C (:zac)]

If there's a test py file listed in the manifest but this file does not exist in the file system then ManifestParser does nothing.

This is similar to bug 849767 in that a typo can easily occur when creating manifests or a file can be moved/renamed without the corresponding manifest being updated.

It would be a nice UX if ManifestParser offered a warning or chuck an Exception so the user realises that they have have a mistake in their manifest file and are possibly unintentionally excluding tests from their test run.

Comment 1

[Dave Hunt [:davehunt] [he/him] ⌚BST]

By default, ManifestDestiny will exclude tests that do not exist [1]. If you want to include these then we should make a change to MarionetteTestRunner [2] to include these tests, and another change to log or raise exceptions when tests do not exist [3].

[1] https://github.com/mozilla/mozbase/blob/master/manifestdestiny/manifestparser/manifestparser.py#L737
[2] http://hg.mozilla.org/mozilla-central/file/c232bec6974d/testing/marionette/client/marionette/runtests.py#l455
[3] http://hg.mozilla.org/mozilla-central/file/c232bec6974d/testing/marionette/client/marionette/runtests.py#l400

Let's determine the behaviour we would like in MarionetteTestRunner/GaiaTestRunner. I don't think this is a ManifestParser issue.

Comment 2

[Zac C (:zac)]

OK thanks for that detail :)

I kind of reasoned that when parsing the manifest it would be the parser that checks the file system for the corresponding python file. But the bug is raised against MozBase anyway!

Comment 3

[Dave Hunt [:davehunt] [he/him] ⌚BST]

MarionetteTestRunner is not part of mozbase. So that we can fix this, what behaviour would you expect when a test file is not present?

Also, we need to be sure that we don't break anything else using MarionetteTestRunner as it's not just the Gaia UI Python tests.

Comment 4

[Zac C (:zac)]

The key thing to me is that when there's a [test_file.py] listed in a manifest file, whether that file exists or is spelt incorrectly or whatever, it is a solid indication of intent by the user. The intention is that there should be a test included in the test run. 

If we assume that if a user is rational and that if they knew about an error in the manifest file then they would immediately fix it. So if an erroneous file reference exists in the manifest file then something is broken between the user's intent in their mind and the actual test run.

Thus I think the test runner should respect the user's intention and try and find/run that py file, even if it leads to the point where it might need to say "look I've tried but I just can't find that file".

At the moment by silently ignoring the test it does not respect the user's intent.

I am completely open to what behaviour occurs. I would prefer an Exception to be thrown, while harsh it does bring attention to the matter immediately so that it can be resolved. In reality a happy resolution to this bug for me would be for it to do anything other than ignore silently.

Comment 5

[Dave Hunt [:davehunt] [he/him] ⌚BST]

I understand, and if you add a reference to an non-existent manifest file you will see an exception. Let's get some feedback on whether such a change would cause issues with other tests using MarionetteTestRunner. If not, I'm happy to work on a patch that will throw an exception if you try to run a test that does not exist.

Comment 6

[Zac C (:zac)]

Thanks Dave that would be great (pending the feedback of course!)

Comment 7

[Jonathan Griffin (:jgriffin)]

(In reply to Dave Hunt (:davehunt) from comment #5)
> I understand, and if you add a reference to an non-existent manifest file
> you will see an exception. Let's get some feedback on whether such a change
> would cause issues with other tests using MarionetteTestRunner. If not, I'm
> happy to work on a patch that will throw an exception if you try to run a
> test that does not exist.

That makes sense to me.  Currently, attempting to run a test that doesn't exist just gives a very unhelpful passed 0/failed 0 result.  Making this throw an exception shouldn't break anything.

Comment 8

[Jeff Hammel]

(In reply to Zac C (:zac) from comment #0)
<snip/>
> This is similar to bug 849767 in that a typo can easily occur when creating
> manifests or a file can be moved/renamed without the corresponding manifest
> being updated.

Not really sure the comparison here...

Comment 9

[Zac C (:zac)]

Sorry that's the wrong bug, but it doesn't matter anyway as we are all in agreeance here.

Comment 10

[Jeff Hammel]

In the process of triaging the next round of manifestparser work. While this bug is not (any longer?) mozbase, Zac is right: we should do something at the manifestparser level here.  I'll comment with more details when I get a bigger picture of all the bugs together; the actual work may be done with another bug.

A few addendum:

(In reply to Zac C (:zac) from comment #4)
<snip/>
> At the moment by silently ignoring the test it does not respect the user's
> intent.

Without going too far afield, the users' intent, plural, is not necessarily in general always satisfiable OOTB.  The initial implementation of manifestparser.py followed the very specific (applicable at the time) intent that if test files did not exist, by default, this meant that the e.g. post-build invocation referred to files e.g. not in the tree. While this is, in retrospect, the wrong intent to standardize on, it is certainly a valid intent and one that we should keep flexibility to address. (OT; it is certainly never my intent to use `git commit` without the `-a` flag but...obviously others have differing intents).

In any case, you're right, Zac, that this default beahviour is a bit dated.  We want to make both ways possible.  Now that manifestdestiny is actually used we need to help this software grow up.  Again, I'll post specifics here after I'm done triaging the next round of work.

> I am completely open to what behaviour occurs. I would prefer an Exception
> to be thrown, while harsh it does bring attention to the matter immediately
> so that it can be resolved. In reality a happy resolution to this bug for me
> would be for it to do anything other than ignore silently.

I also favor failing fast and loudly when it is a failure.  The issue here is: currently, it is not a defined failure for manifestparser.py

Anyway, will sort more of this out and report back.

Comment 11

[Dave Hunt [:davehunt] [he/him] ⌚BST]

+1 for failing fast, which would mean fixing this in the parser.

Comment 12

[Zac C (:zac)]

That's great, thanks jhammel!

Comment 13

[Andrew Halberstadt [:ahal]]

Huh, I thought we did this on strict already, but apparently not:
http://mxr.mozilla.org/mozilla-central/source/testing/mozbase/manifestdestiny/manifestparser/manifestparser.py#791

Comment 14

[Zac C (:zac)]

I think we got strict on this in MarionetteTestRunner but not in MozRunner itself.

Comment 15

[Andrew Halberstadt [:ahal]]

Attachment: Error out or print warning if test doesn't exist

I assume you mean manifestparser? :)

I think this patch is what we're looking for.. we'll need to do a try push in case there are missing test paths in mochitest or xpcshell.

Comment 16

[Andrew Halberstadt [:ahal]]

Try run:
https://tbpl.mozilla.org/?tree=Try&rev=ddc9f55c71a1

Comment 17

[Andrew Halberstadt [:ahal]]

Attachment: Error out or print warning if test doesn't exist

Oh, I forgot to add a test file. Here's the updated patch with new try run:
https://tbpl.mozilla.org/?tree=Try&rev=c00a33d2eaaf

Comment 18

[Zac C (:zac)]

Ahal, is a checkin-needed here?

Comment 19

[Andrew Halberstadt [:ahal]]

Yikes, that sucks.. not sure how I missed this. We'll need to do another full try run first to make sure there weren't any missing manifests added in the interim.

Comment 20

[Andrew Halberstadt [:ahal]]

s/missing manifests/missing tests

https://tbpl.mozilla.org/?tree=Try&rev=10adf71ad75b

Comment 21

[Andrew Halberstadt [:ahal]]

There's a non-existent mozbase test, but looks like that's it. Wlach said he would fix it in bug 1016467.

Comment 22

[Andrew Halberstadt [:ahal]]

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

Sorry this took so long. Just a note in case anyone is confused why their applications aren't throwing on non-existent tests.. The parser needs to be created with 'strict=True' for this behaviour to kick in.

Comment 23

[Carsten Book [:Tomcat]]

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