Closed Bug 1208050 Opened 9 years ago Closed 8 years ago

Luciddream should support luciddream.ini test manifests, like other harness

Categories

(Testing :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED INCOMPLETE

People

(Reporter: ochameau, Assigned: ochameau)

References

Details

Attachments

(1 file, 4 obsolete files)

Today, on automation, luciddream runs against a single ini file: http://mxr.mozilla.org/mozilla-central/source/testing/mozharness/scripts/luciddream_unittest.py#240 It prevents sheriffing this test suite sanely and doesn't help adding new tests. Ideally luciddream would support luciddream.in test manifests, like what we do for other test suites. I have a patch that tweak ./mach luciddream to do that. Support manifests and run them all if no argument is given. But this mozharness code doesn't run luciddream via mach. That would be great to run them via the same command than developers, rather than calling luciddream manually like this. But if that's too complex, I can also move my logic from mach command to luciddream.
Attached patch patch v1 (obsolete) — Splinter Review
While submitting this patch I realized that yes, it will surely break mozharness. I'll address that one way or another, but I would like to get some feedback first. Also I modified various mozbuild file, just to match what had been done for other harness, but I'm not sure everything is required?
Attached patch patch v2 (obsolete) — Splinter Review
Removed some useless modification in mozbuild, also do not require objdir if we only pass explicit paths to test files (we need an objdir if we pass manifest file, or folder, or nothing in order to compute list of tests according to manifest and skip-if rules). At the end, I think it makes sense to depend on ./mach. We will need a mozconfig to handle correctly the luciddream.ini files. It's not clear to me if luciddream_unittest.py has still access to the gecko directory used for the build. I imagine it is independant and doesn't have access to it. But it may have access to a mozconfig? If that helps, I used to have a very simple taskcluster manifest to run luciddream over ./mach. But I have the same limitation, I don't have access to the build/objdir, and I don't think I have access to a mozconfig either on TC. Ted, could you give me some feedback about this? I think I can already land this patch. The manifest are going to work locally. It is only matter to make it work on automation, that can be done as a followup. See also bug 1208057 where I'm tweaking luciddream to be able to run js files directly.
Attachment #8665414 - Attachment is obsolete: true
Attachment #8666728 - Flags: feedback?
Attachment #8666728 - Flags: feedback? → feedback?(ted)
Attached patch patch v3 (obsolete) — Splinter Review
Rebased.
Attachment #8666728 - Attachment is obsolete: true
Attachment #8666728 - Flags: feedback?(ted)
Attachment #8667200 - Flags: feedback?(ted)
Attached patch patch v4 (obsolete) — Splinter Review
Attachment #8667200 - Attachment is obsolete: true
Attachment #8667200 - Flags: feedback?(ted)
Comment on attachment 8668487 [details] [diff] [review] patch v4 This patch shouldn't break existing lucciddream mozharness script, but still, doesn't make it run all luciddream.ini file.
Attachment #8668487 - Flags: feedback?(ted)
Assignee: nobody → poirot.alex
Attached patch patch v5Splinter Review
Patch with green try! Considering this patch as ready for production and review. Ted, please forward to whoever makes sense if you are not good candidate to review such patch and/or don't have time to review.
Attachment #8668487 - Attachment is obsolete: true
Attachment #8668487 - Flags: feedback?(ted)
Attachment #8669645 - Flags: review?(ted)
Jonathan, Do you know who could help review this patch? I had no luck with :ted so far.
Flags: needinfo?(jgriffin)
Comment on attachment 8669645 [details] [diff] [review] patch v5 Review of attachment 8669645 [details] [diff] [review]: ----------------------------------------------------------------- Chris, can you review the build system parts of this? I can review the luciddream parts, or you can review the whole thing if you'd like.
Attachment #8669645 - Flags: review?(ted)
Attachment #8669645 - Flags: review?(jgriffin)
Attachment #8669645 - Flags: review?(cmanchester)
Flags: needinfo?(jgriffin)
Comment on attachment 8669645 [details] [diff] [review] patch v5 Review of attachment 8669645 [details] [diff] [review]: ----------------------------------------------------------------- ::: python/mozbuild/mozbuild/testing.py @@ +259,5 @@ > BROWSER_CHROME=('browser-chrome', 'testing/mochitest', 'browser', True), > ANDROID_INSTRUMENTATION=('instrumentation', 'instrumentation', '.', False), > JETPACK_PACKAGE=('jetpack-package', 'testing/mochitest', 'jetpack-package', True), > JETPACK_ADDON=('jetpack-addon', 'testing/mochitest', 'jetpack-addon', False), > + LUCIDDREAM=('luciddream', 'testing/mochitest', 'luciddream', True), This ends up installing the manifest to $objdir/_tests/testing/mochitest/luciddream. The 'testing/mochitest' prefix seems spurious. ::: testing/luciddream/mach_commands.py @@ +90,5 @@ > + all(os.path.exists(path) and \ > + os.path.splitext(path)[1] in [".js", ".py"] \ > + for path in test_paths) > + > + if not explicitPaths: nit: Explicit line continuations (the "\") can usually be omitted when there are parentheses around to disambiguate. So this might look a little cleaner without the last two. @@ +103,5 @@ > + manifest.tests.extend(test_paths) > + import mozinfo > + > + # Accept disabled path only when we explicitely pass disabled file > + tests = manifest.active_tests(disabled=False, **mozinfo.info) I'm not sure about this comment. It looks like disabled tests aren't going to be run no matter what file is passed (I'm not sure it matters at this juncture). ::: testing/luciddream/moz.build @@ +1,5 @@ > +# This Source Code Form is subject to the terms of the Mozilla Public > +# License, v. 2.0. If a copy of the MPL was not distributed with this > +# file, You can obtain one at http://mozilla.org/MPL/2.0/. > + > +LUCIDDREAM_MANIFESTS += ['example-tests/luciddream.ini'] This isn't going to make it into the build -- the directory containing the moz.build needs to be mentioned in a parent moz.build (other entries for testing/ are in toolkit/toolkit.mozbuild). ::: testing/mach_commands.py @@ +140,5 @@ > }, > + 'luciddream': { > + 'mach_command': 'luciddream', > + 'kwargs': {'test_paths': []}, > + }, This section is so tests can be run with the |./mach test| command, but this isn't going to work as is, because the run_luciddream_test function will end up getting called with exactly these keyword args, while it requires browser_path, b2g_desktop_path, etc. positional args. I see a lot of mach commands in the tree that have only keyword args to accommodate this.
Attachment #8669645 - Flags: review?(jgriffin)
Attachment #8669645 - Flags: review?(cmanchester)
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: