Open
Bug 997744
Opened 11 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.
Categories
(Testing :: Mochitest, defect)
Tracking
(Not tracked)
NEW
People
(Reporter: kushagra, Unassigned)
Details
Attachments
(1 file, 3 obsolete files)
1.78 KB,
patch
|
Details | Diff | Splinter Review |
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.
Updated•11 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Reporter | ||
Comment 1•11 years ago
|
||
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 2•11 years ago
|
||
Comment on attachment 8410266 [details] [diff] [review]
997744fix_V1.patch
Not it.
Attachment #8410266 -
Flags: review?(Ms2ger) → review?(ted)
Reporter | ||
Updated•11 years ago
|
Assignee: nobody → singh.kushagra93
Comment 3•11 years ago
|
||
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.
Reporter | ||
Updated•11 years ago
|
Status: NEW → ASSIGNED
Comment 4•11 years ago
|
||
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'
Attachment #8410266 -
Flags: review?(ted) → review-
Reporter | ||
Updated•11 years ago
|
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.
Reporter | ||
Comment 5•11 years ago
|
||
Made the required changes. Works even if xpcshell-tests are passed.
Attachment #8410266 -
Attachment is obsolete: true
Attachment #8427984 -
Flags: review?(ted)
Comment 6•11 years ago
|
||
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.
Attachment #8427984 -
Flags: review?(ted) → review-
Reporter | ||
Comment 7•11 years ago
|
||
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 8•11 years ago
|
||
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 !=
Attachment #8430245 -
Flags: review?(ted) → review+
Reporter | ||
Comment 9•11 years ago
|
||
Attachment #8430245 -
Attachment is obsolete: true
Reporter | ||
Comment 11•10 years ago
|
||
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)
Comment 13•3 years ago
|
||
The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.
Assignee: singh.kushagra93 → nobody
Status: ASSIGNED → NEW
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•