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

RESOLVED FIXED in mozilla35

Status

RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: zcampbell, Assigned: ahal)

Tracking

unspecified
mozilla35
Other
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mozbase][runner])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

6 years ago
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.
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.
(Reporter)

Comment 2

6 years ago
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!
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.
Component: Mozbase → Marionette
QA Contact: hskupin
Summary: If py file in a manifest file does not exist, manifestparser does nothing → Tests referenced by manifest files that do not exist are silently ignored
(Reporter)

Comment 4

6 years ago
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.
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.
(Reporter)

Comment 6

6 years ago
Thanks Dave that would be great (pending the feedback of course!)
(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

6 years ago
(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...
(Reporter)

Comment 9

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

Comment 10

6 years ago
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.
Whiteboard: [mozbase]
+1 for failing fast, which would mean fixing this in the parser.
(Reporter)

Comment 12

6 years ago
That's great, thanks jhammel!

Updated

6 years ago
Depends on: 860499
Whiteboard: [mozbase] → [mozbase][runner]
(Reporter)

Comment 14

5 years ago
I think we got strict on this in MarionetteTestRunner but not in MozRunner itself.
(Assignee)

Comment 15

5 years ago
Created attachment 8398557 [details] [diff] [review]
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.
Assignee: nobody → ahalberstadt
Status: NEW → ASSIGNED
(Assignee)

Comment 17

5 years ago
Created attachment 8398745 [details] [diff] [review]
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
Attachment #8398557 - Attachment is obsolete: true
Attachment #8398745 - Flags: review?(dave.hunt)
Attachment #8398745 - Flags: review?(dave.hunt) → review+
(Reporter)

Comment 18

4 years ago
Ahal, is a checkin-needed here?
Flags: needinfo?(ahalberstadt)
(Assignee)

Comment 19

4 years ago
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.
Flags: needinfo?(ahalberstadt)
(Assignee)

Comment 20

4 years ago
s/missing manifests/missing tests

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

Comment 21

4 years ago
There's a non-existent mozbase test, but looks like that's it. Wlach said he would fix it in bug 1016467.
Depends on: 1016467
(Assignee)

Comment 22

4 years ago
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.
https://hg.mozilla.org/mozilla-central/rev/bcd68e6935ba
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.