Open Bug 997744 Opened 10 years ago Updated 2 years ago

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


(Testing :: Mochitest, defect)



(Not tracked)


(Reporter: kushagra, Unassigned)



(1 file, 3 obsolete files)

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.
Ever confirmed: true
Attached patch 997744fix_V1.patch (obsolete) — Splinter Review
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.
Attachment #8410266 - Flags: review?(Ms2ger)
Comment on attachment 8410266 [details] [diff] [review]

Not it.
Attachment #8410266 - Flags: review?(Ms2ger) → review?(ted)
Assignee: nobody → singh.kushagra93
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 on attachment 8410266 [details] [diff] [review]

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

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

::: testing/mochitest/
@@ +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):

I think what you actually want is run_desktop_test:

You can see the error message it prints here:

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']
Attachment #8410266 - Flags: review?(ted) → review-
Summary: In case of a mochitest-chrome test, mochitest-plain should inform that it's a mochitest-chrome test but it gives a |Specified test path does not exist| error. → Mochitest should inform if the target test suite and the suite of the test being executed is not the same.
Attached patch removing the path manipulations. (obsolete) — Splinter Review
Made the required changes. Works even if xpcshell-tests are passed.
Attachment #8410266 - Attachment is obsolete: true
Attachment #8427984 - Flags: review?(ted)
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/
@@ +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.
Attachment #8427984 - Flags: review?(ted) → review-
Attached patch ditched the try:except approach (obsolete) — Splinter Review
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.
Attachment #8427984 - Attachment is obsolete: true
Attachment #8430245 - Flags: review?(ted)
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/
@@ +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 !=
Attachment #8430245 - Flags: review?(ted) → review+
Attached patch Final patch.Splinter Review
Attachment #8430245 - Attachment is obsolete: true
Flags: needinfo?(singh.kushagra93)
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.
Flags: needinfo?(singh.kushagra93) → needinfo?(Ms2ger)
Anything you need from me here?
Flags: needinfo?(Ms2ger)

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.

Assignee: singh.kushagra93 → nobody
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.