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

Status: RESOLVED INCOMPLETE

(Testing :: General, defect)

Description

[Alexandre Poirot [:ochameau]]

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.

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: patch v1

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?

Comment 2

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7833a57ff13b

Comment 3

[Alexandre Poirot [:ochameau]]

Attachment: patch v2

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.

Comment 4

[Alexandre Poirot [:ochameau]]

Attachment: patch v3

Rebased.

Comment 5

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9765ad3b8320

Comment 6

[Alexandre Poirot [:ochameau]]

Attachment: patch v4

Comment 7

[Alexandre Poirot [:ochameau]]

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.

Comment 8

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=82bdcc356234

Comment 9

[Alexandre Poirot [:ochameau]]

Attachment: patch v5

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.

Comment 10

[Alexandre Poirot [:ochameau]]

Jonathan, Do you know who could help review this patch?
I had no luck with :ted so far.

Comment 11

[Jonathan Griffin (: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.

Comment 12

[Chris Manchester (limited bugmail, email directly)]

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.