Open
Bug 997744
Opened 7 years ago
Updated 6 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)
ASSIGNED
People
(Reporter: kushagra, Assigned: kushagra)
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•7 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee | ||
Comment 1•7 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•7 years ago
|
||
Comment on attachment 8410266 [details] [diff] [review] 997744fix_V1.patch Not it.
Attachment #8410266 -
Flags: review?(Ms2ger) → review?(ted)
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → singh.kushagra93
Comment 3•7 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.
Assignee | ||
Updated•7 years ago
|
Status: NEW → ASSIGNED
Comment 4•7 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-
Assignee | ||
Updated•7 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.
Assignee | ||
Comment 5•7 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•7 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-
Assignee | ||
Comment 7•7 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•7 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+
Assignee | ||
Comment 9•7 years ago
|
||
Attachment #8430245 -
Attachment is obsolete: true
Assignee | ||
Comment 11•6 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)
You need to log in
before you can comment on or make changes to this bug.
Description
•