1237958 - Running marionette via mach attempts to run the wrong things if a manifest isn't specified

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[Mark Banner (:standard8)]

I was helping another developer who was trying to run the marionette tests but was getting failures. We determined that it was because he was doing:

./mach marionette-test browser/extensions/loop

rather than:

./mach marionette-test browser/extensions/loop/manifest.ini

The first form attempts to run some random tests in the directory, rather than the actual tests.

xpcshell & mochitest work without specifying the manifest, it would be nice if marionette did here as well - or at least gave a proper error.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Mark Banner (:standard8) from comment #0)
> ./mach marionette-test browser/extensions/loop
[..] 
> The first form attempts to run some random tests in the directory, rather
> than the actual tests.

Really "some" random tests? If you specify a folder all the files starting with test_* should be run. About the default order I'm not sure.

Comment 2

[Mark Banner (:standard8)]

(In reply to Henrik Skupin (:whimboo) from comment #1)
> (In reply to Mark Banner (:standard8) from comment #0)
> > ./mach marionette-test browser/extensions/loop
> [..] 
> > The first form attempts to run some random tests in the directory, rather
> > than the actual tests.
> 
> Really "some" random tests? If you specify a folder all the files starting
> with test_* should be run. About the default order I'm not sure.

Well, its probably trying to run the xpcshell ones then, which I'd call random. The way the other commands work is if you specify a directory, they'll run everything under that sub-directory, but only those that are matching the test types as supplied by the manifest.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

So the test argument gets passed-through to Marionette itself:
http://mxr.mozilla.org/mozilla-central/source/testing/marionette/mach_commands.py#52

That means the same happens when running Marionette itself. As far as I remember Marionette explicitly expects a manifest to be given for a folder, otherwise it iterates through all sub folders for possible tests. This is happening here. The manifest has tests which point to a complete different directory.

http://mxr.mozilla.org/mozilla-central/source/browser/extensions/loop/
http://mxr.mozilla.org/mozilla-central/source/browser/extensions/loop/manifest.ini

Andreas or David, not sure if that is what we want for Marionette or not regarding parity with other test harnesses.

Comment 4

[Andreas Tolfsen ❲:ato❳]

I have no knowledge of how manifests or the Marionette Python harness works.

Comment 5

[David Burns :automatedtester]

we should probably have parity here so that people get what they want.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I wonder how other harnesses work in case no manifest.ini file has been found in the specified directory. Do they also collect all available files starting with test_ or do nothing?

Comment 7

[Anjana Vakil (:vakila)]

As Henrik said, if a directory is specified, Marionette will walk the directory and add any test files (files named "test_*(.py|.js)"). It will just ignore a ".ini" manifest if there's one in that directory. But it seems it would be pretty simple to have it first look for a manifest (".ini") in the specified directory (or look through all subdirectories recursively?), and if there is one, add the tests in that (as if you had passed the manifest to the command). 

Maja, is that something I should do? Or do we want to leave it as-is for now? Either way, for Bug 1276974 I'll test that the expected behavior is happening.

Comment 8

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Not sure. What if you actually want to run all the tests in a directory that contains a manifest? It sucks to have to edit the manifest to be able to do that.

tl;dr: matching the behaviour of some other mach commands (by using TestResolver) is a larger question/task, and for now I think we should just clarify the |mach marionette-test| output/help message/options so that users don't get confused. 

The mochitest mach command uses TestResolver [1], which only runs tests that are listed in manifests referenced in moz.build files [2] and depends on ./mach configure. When I was working on |mach python-test|, I actually found this approach quite opaque (not clear which tests or test manifests will be run) and inflexible (can't easily run tests that are not in recognized manifest). I ended up added a --path-only option to |mach python-test| in response (Bug 1261412).


[1] https://dxr.mozilla.org/mozilla-central/source/testing/mochitest/mach_commands.py#158
[2] https://dxr.mozilla.org/mozilla-central/source/devtools/client/shared/components/moz.build#20

Comment 9

[Anjana Vakil (:vakila)]

(In reply to Maja Frydrychowicz (:maja_zf) from comment #8)
> tl;dr: matching the behaviour of some other mach commands (by using
> TestResolver) is a larger question/task, and for now I think we should just
> clarify the |mach marionette-test| output/help message/options so that users
> don't get confused. 

In addition to clarifying in the help message, what if, when a user specifies the path to a dir, we do check for an `.ini` file, and if found, we display a message like the following?

"Dir containing manifest specified; ignoring `manifest.ini` and running all tests found in `browser/extensions/loop`. 
To run manifest tests use the command `./mach marionette-test browser/extensions/loop/manifest.ini`. 
See command help for details."

Comment 10

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

That sounds good.

Comment 11

[Anjana Vakil (:vakila)]

Attachment: Bug 1237958 - Expand `mach marionette-test` help msg re test paths;

Expand the help message for the `marionette-test` mach command
to more clearly explain Marionette's behavior when a path to
a test module, directory, or manifest file is specified.

Review commit: https://reviewboard.mozilla.org/r/58232/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58232/

Comment 12

[Anjana Vakil (:vakila)]

Attachment: Bug 1237958 - Clarify Marionette's behavior re dir/manifest;

If a user passes a directory path to Marionette on the command line,
any manifest ('.ini') file(s) in that dir will be ignored and
instead all test modules in the dir will be run. Clarify this
behavior by logging a warning if we find any '.ini' file(s)
in the specified directory.

Review commit: https://reviewboard.mozilla.org/r/58234/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58234/

Comment 13

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8760780 [details]
Bug 1237958 - Clarify Marionette's behavior re dir/manifest;

https://reviewboard.mozilla.org/r/58234/#review55204

::: testing/marionette/harness/marionette/runner/base.py:900
(Diff revision 1)
>  
>          if os.path.isdir(filepath):
>              for root, dirs, files in os.walk(filepath):
>                  for filename in files:
> -                    if (filename.startswith('test_') and
> +                    if (filename.endswith('.ini')):
> +                        relpath = os.path.relpath(os.path.join(root, filename), test)

I'm guessing that it might be better to use `filepath` here and below instead of `test`

::: testing/marionette/harness/marionette/runner/base.py:901
(Diff revision 1)
> +                        msg_tmpl = "Directory containing manifest specified; "\
> +                                  "ignoring manifest '{0}' and running all tests found in '{1}'. "\
> +                                  "To run the manifest tests specify the manifest file directly. "\
> +                                  "See command help for details."

Let's shorten this message to something like "Ignoring manifest '{0}' and running all tests in '{1}'. See --help for details."

::: testing/marionette/harness/marionette/runner/base.py:901
(Diff revision 1)
> +                        msg_tmpl = "Directory containing manifest specified; "\
> +                                  "ignoring manifest '{0}' and running all tests found in '{1}'. "\
> +                                  "To run the manifest tests specify the manifest file directly. "\
> +                                  "See command help for details."

Instead of using line breaks '\' please wrap the long string in () like:
```
("my "
 "multi-line "
 "string")
```

Comment 14

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8760779 [details]
Bug 1237958 - Expand `mach marionette-test` help msg re test paths;

https://reviewboard.mozilla.org/r/58232/#review55210

::: testing/marionette/harness/marionette/runner/base.py:256
(Diff revision 1)
> +                               'Can be path(s) to Python or JavaScript test modules, '
> +                               'the path to a directory containing test modules, '
> +                               "or the path to a manifest ('.ini') file describing tests to run. "
> +                               'If the path to a directory is provided and that directory '
> +                               'contains both test modules and a manifest file, '
> +                               'the manifest will be ignored and all test modules '
> +                               'in the given directory will be run.')

Yay for clear help messages! :) Could you make the new part shorter?
Something like "One or more paths to test files (Python or JS), manifest files (ini) or directories. When a directory is specified, all test files in the directory will be run."

Comment 15

[Anjana Vakil (:vakila)]

Comment on attachment 8760779 [details]
Bug 1237958 - Expand `mach marionette-test` help msg re test paths;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58232/diff/1-2/

Comment 16

[Anjana Vakil (:vakila)]

Comment on attachment 8760780 [details]
Bug 1237958 - Clarify Marionette's behavior re dir/manifest;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58234/diff/1-2/

Comment 17

[Anjana Vakil (:vakila)]

https://reviewboard.mozilla.org/r/58234/#review55332

::: testing/marionette/harness/marionette/runner/base.py:903
(Diff revisions 1 - 2)
> -                                  "To run the manifest tests specify the manifest file directly. "\
> +                        self.logger.warning(msg_tmpl.format(relpath, filepath))
> -                                  "See command help for details."
> -                        self.logger.warning(msg_tmpl.format(relpath, test))
>                      elif (filename.startswith('test_') and
>                          (filename.endswith('.py') or filename.endswith('.js'))):
> -                        filepath = os.path.join(root, filename)
> +                        test_file = os.path.join(root, filename)

The variable `filepath` was being used for two different things. Renamed the second usage to `test_file` to avoid conflict.

Comment 18

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8760780 [details]
Bug 1237958 - Clarify Marionette's behavior re dir/manifest;

https://reviewboard.mozilla.org/r/58234/#review55446

Looks good. In this case, squash down to one commit please.

Comment 19

[Anjana Vakil (:vakila)]

Comment on attachment 8760780 [details]
Bug 1237958 - Clarify Marionette's behavior re dir/manifest;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58234/diff/2-3/

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/17dec170c383
Clarify Marionette's behavior re dir/manifest; r=maja_zf

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/17dec170c383