997744 - Mochitest should inform if the target test suite and the suite of the test being executed is not the same.

Status: ASSIGNED

(Testing :: Mochitest, defect)

Description

[Kushagra Singh [:kushagra]]

When running a mochitest-chrome test, mochitest-plain gives a |Specified test path does not exist| error. Instead it should tell the user that the test is a mochitest-chrome test and not a -plain test.

Comment 1

[Kushagra Singh [:kushagra]]

Attachment: 997744fix_V1.patch

I could not find a way to directly get the flavor of the test passed as argument. If anyone can point out a method, then great. Meanwhile, i used file path manipulation to get the name of the manifest file in that test directory.

Assuming that the manifest file is properly defined, it's name will the give the flavor of the test. For example, mochitest-chrome tests will have a chrome.ini manifest file.

This flavor then can be compared to the suite of the target mochitest and the correct error message could be displayed.

This approach needs to tested on non OS X systems.

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 8410266 [details] [diff] [review]
997744fix_V1.patch

Not it.

Comment 3

[Manish Goregaokar [:manishearth]]

Firstly, it might be worthwhile to catch mochitest-browser (and there's this newfangled mochitest-devtools as well.). The patch seems to do this, but I'm not sure.

Secondly, it probably should go ahead and run the test in the other suite, maybe after asking the user if they want to do so.

Comment 4

[(not currently active) Ted Mielczarek]

Comment on attachment 8410266 [details] [diff] [review]
997744fix_V1.patch

Review of attachment 8410266 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch, sorry about the delay in the review!

::: testing/mochitest/mach_commands.py
@@ +331,5 @@
>  
>          if test_path:
>              test_root = runner.getTestRoot(options)
>              test_root_file = mozpack.path.join(self.mochitest_dir, test_root, test_path)
> +            file_dir = self.mochitest_dir[:self.mochitest_dir.find('obj')] + os.path.dirname(test_path)

The function you're changing here is run_b2g_test, which is only responsible for running Mochitests on B2G (Firefox OS):
http://hg.mozilla.org/mozilla-central/annotate/80ab72ae4351/testing/mochitest/mach_commands.py#l134

I think what you actually want is run_desktop_test:
http://hg.mozilla.org/mozilla-central/annotate/80ab72ae4351/testing/mochitest/mach_commands.py#l186

You can see the error message it prints here:
http://hg.mozilla.org/mozilla-central/annotate/80ab72ae4351/testing/mochitest/mach_commands.py#l332

Also, I don't think you should have to do path manipulation here. If you look at the links above, you can see that it has a TestResolver object. Currently it calls "resolve_tests(paths=test_paths, flavor=flavor, cwd=context.cwd)" and errors if this doesn't return any tests. You ought to be able to call resolve_tests again in that case but leave out the "flavor", argument, and if you get a non-empty list the results will have a "flavor" in the result. You can try this out on the commandline like so:

$ ./mach python
Python 2.7.6 (default, Mar 22 2014, 22:59:56) 
[GCC 4.8.2] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> from mozbuild.testing import TestResolver
>>> from mozbuild.base import MozbuildObject
>>> e = MozbuildObject.from_environment()
>>> t = e._spawn(TestResolver)
>>> tests = list(t.resolve_tests(paths=['dom/tests/mochitest/gamepad/test_gamepad.html']))
>>> x = tests[0]
>>> x['flavor']
u'mochitest'

Comment 5

[Kushagra Singh [:kushagra]]

Attachment: removing the path manipulations.

Made the required changes. Works even if xpcshell-tests are passed.

Comment 6

[(not currently active) Ted Mielczarek]

Comment on attachment 8427984 [details] [diff] [review]
removing the path manipulations.

Review of attachment 8427984 [details] [diff] [review]:
-----------------------------------------------------------------

Good start! Just a little bit of rework and this will be in good shape.

::: testing/mochitest/mach_commands.py
@@ +338,5 @@
> +                            'and the suite of the test being executed is the same.')
> +                except:
> +                    print('No tests could be found in the path specified. Please '
> +                        'specify a path that is a test file or is a directory '
> +                        'containing tests.')

I would avoid using a try/except here, and just repeat the "if not tests" check again, like:
tests = list(...)
if not tests:
  print('No tests could be found...')
  return 1
test_flavor = tests[0]
...

You should also print the actual test flavor in the error message since you know it and it tells the developer what they need to know.

Comment 7

[Kushagra Singh [:kushagra]]

Attachment: ditched the try:except approach

Thanks for the tip. It's neat.
I also noticed that this way we don't even need to check if the flavor and the suite are same or not, since as far as we know, only wrong test-path or the wrong suite are the ways to throw an error. So if it's not the wrong path name then it's definitely the wrong suite.
Though checking the flavor is logical, as now we have explicitly stated in the code that there are only two ways to cause an error we have to make sure that there is no third way.

Comment 8

[(not currently active) Ted Mielczarek]

Comment on attachment 8430245 [details] [diff] [review]
ditched the try:except approach

Review of attachment 8430245 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good, just one tiny formatting nit. Thanks!

::: testing/mochitest/mach_commands.py
@@ +336,5 @@
> +                        'specify a path that is a test file or is a directory '
> +                        'containing tests.')
> +                    return 1
> +                test_flavor = test[0]
> +                if test_flavor['flavor']!= suite:

nit: put a space in front of the !=

Comment 9

[Kushagra Singh [:kushagra]]

Attachment: Final patch.

Comment 10

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Status?

Comment 11

[Kushagra Singh [:kushagra]]

Extremely sorry for the late reply :( Kinda went off the grid. The patch was ready to commit back then but now I guess it will take some more work because of the changes in the codebase.

Comment 12

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Anything you need from me here?