Closed Bug 1205687 Opened 4 years ago Closed 4 years ago

Add support for Marionette tests to `mach test` target

Categories

(Testing :: General, defect)

defect
Not set

Tracking

(firefox46 fixed)

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: ato, Assigned: ato)

Details

(Keywords: pi-marionette-runner)

Attachments

(5 files)

`mach test FILE` should be able to deduce that FILE is a Marionette test and call the steps associated with `mach marionette-test`.
Assignee: nobody → ato
Status: NEW → ASSIGNED
Component: mach → General
Product: Core → Testing
Bug 1205687: Package and include Marionette tests in all-tests.json

Listing the tests in all-tests.json is prerequisite to adding support
for Marionette tests to `./mach test FILE`.

r=ted
Attachment #8663603 - Flags: review?(ted)
Bug 1205687: Add Mn test flavour and include tests in build manifests

r=ted
Attachment #8663604 - Flags: review?(ted)
Bug 1205687: Add metadata about functional Loop tests

r=Standard8
Attachment #8663605 - Flags: review?(standard8)
Bug 1205687: Add metadata about Marionette layout tests

r=Ms2ger
Attachment #8663606 - Flags: review?(Ms2ger)
Bug 1205687: Add metadata about Marionette WebAPI tests

This patch also renames certain test files to avoid
duplication of test names.  Because they are all included in
testing/marionette/client/marionette/tests/webapi-tests.ini through
include statements, the test names must be unique.

r=echen
Attachment #8663607 - Flags: review?(echen)
Comment on attachment 8663605 [details]
MozReview Request: Bug 1205687 - Add metadata about functional Loop tests; r?standard8

https://reviewboard.mozilla.org/r/19683/#review17831

I've not tested it, but it looks fine to me for the change.
Attachment #8663605 - Flags: review?(standard8) → review+
Attachment #8663606 - Flags: review?(Ms2ger) → review+
Comment on attachment 8663606 [details]
MozReview Request: Bug 1205687 - Add metadata about Marionette layout tests; r?ms2ger

https://reviewboard.mozilla.org/r/19685/#review17835
Comment on attachment 8663607 [details]
MozReview Request: Bug 1205687 - Add metadata about Marionette WebAPI tests; r?edgar r?shawnjohnjr

https://reviewboard.mozilla.org/r/19687/#review17909

r+ for the changes in mobilemessage, telephony and voicemail
Attachment #8663607 - Flags: review?(echen) → review+
Comment on attachment 8663607 [details]
MozReview Request: Bug 1205687 - Add metadata about Marionette WebAPI tests; r?edgar r?shawnjohnjr

Invite Shawn for the review for bluetooth changes.
Attachment #8663607 - Flags: review?(shuang)
Comment on attachment 8663607 [details]
MozReview Request: Bug 1205687 - Add metadata about Marionette WebAPI tests; r?edgar r?shawnjohnjr

Bug 1205687: Add metadata about Marionette WebAPI tests

This patch also renames certain test files to avoid
duplication of test names.  Because they are all included in
testing/marionette/client/marionette/tests/webapi-tests.ini through
include statements, the test names must be unique.

r=echen
https://reviewboard.mozilla.org/r/19679/#review19009

::: python/mozbuild/mozbuild/frontend/context.py:1376
(Diff revision 1)
> +    'MARIONETTE_LAYOUT_MANIFESTS': (StrictOrderingOnAppendList, list,
> +        """List of manifest files defining marionette-layout tests.
> +        """, None),
> +
> +    'MARIONETTE_LOOP_MANIFESTS': (StrictOrderingOnAppendList, list,
> +        """List of manifest files defining marionette-loop tests.
> +        """, None),
> +
> +    'MARIONETTE_UNIT_MANIFESTS': (StrictOrderingOnAppendList, list,
> +        """List of manifest files defining marionette-unit tests.
> +        """, None),
> +
> +    'MARIONETTE_UPDATE_MANIFESTS': (StrictOrderingOnAppendList, list,
> +        """List of manifest files defining marionette-update tests.
> +        """, None),
> +
> +    'MARIONETTE_WEBAPI_MANIFESTS': (StrictOrderingOnAppendList, list,
> +        """List of manifest files defining marionette-webapi tests.
> +        """, None),

Drive by: could we get by with fewer entries here? It doesn't look like the various types need to be handled differently in the current set up.
https://reviewboard.mozilla.org/r/19679/#review19009

> Drive by: could we get by with fewer entries here? It doesn't look like the various types need to be handled differently in the current set up.

You’re right, but this reflects where the tests live in the tree, which we probably want to reflect also in packaging.

This might also be a good starting point for how we want to split up the Mn job in the future.
Comment on attachment 8663603 [details]
MozReview Request: Bug 1205687 - Package and include Marionette tests in all-tests.json; r?chmanchester r?gps

Bug 1205687: Package and include Marionette tests in all-tests.json

Listing the tests in all-tests.json is prerequisite to adding support
for Marionette tests to `./mach test FILE`.

r=ted
Attachment #8663603 - Flags: review?(ted) → review?(james)
Comment on attachment 8663604 [details]
MozReview Request: Bug 1205687 - Add Mn test flavour and include tests in build manifests; r?jgraham

Bug 1205687: Add Mn test flavour and include tests in build manifests

r=ted
Attachment #8663604 - Flags: review?(ted) → review?(james)
Comment on attachment 8663603 [details]
MozReview Request: Bug 1205687 - Package and include Marionette tests in all-tests.json; r?chmanchester r?gps

https://reviewboard.mozilla.org/r/19679/#review20109

I think this is OK, but the comment in emitter.py really wants build peer sign off on changes here, so I suggest you r? gps
Attachment #8663603 - Flags: review?(james)
Comment on attachment 8663604 [details]
MozReview Request: Bug 1205687 - Add Mn test flavour and include tests in build manifests; r?jgraham

https://reviewboard.mozilla.org/r/19681/#review20115

::: testing/marionette/moz.build:11
(Diff revision 1)
> +MARIONETTE_UNIT_MANIFESTS += ['client/marionette/tests/unit/unit-tests.ini']

This doesn't actually work without the last patch in the series, does it?
Attachment #8663604 - Flags: review?(james) → review+
Comment on attachment 8663603 [details]
MozReview Request: Bug 1205687 - Package and include Marionette tests in all-tests.json; r?chmanchester r?gps

Bug 1205687: Package and include Marionette tests in all-tests.json

Listing the tests in all-tests.json is prerequisite to adding support
for Marionette tests to `./mach test FILE`.

r=ted
Attachment #8663603 - Flags: review?(gps)
Comment on attachment 8663603 [details]
MozReview Request: Bug 1205687 - Package and include Marionette tests in all-tests.json; r?chmanchester r?gps

https://reviewboard.mozilla.org/r/19679/#review20171

I'm granting review. But I don't have time to verify this is doing the correct thing before I leave for the airport. I might have time to look at it tomorrow or Thursday. In lieux of me, chmanchester can verify. As module owner, I grant him review privileges in build system code related to test integration.

::: python/mozbuild/mozbuild/frontend/emitter.py:1006
(Diff revision 1)
> +            MARIONETTE_LAYOUT=('marionette', 'marionette', '.', True),
> +            MARIONETTE_LOOP=('marionette', 'marionette', '.', True),
> +            MARIONETTE_UNIT=('marionette', 'marionette', '.', True),
> +            MARIONETTE_UPDATE=('marionette', 'marionette', '.', True),
> +            MARIONETTE_WEBAPI=('marionette', 'marionette', '.', True),

I'm pretty sure the shared "." here means that tests will all get copied to the same destination directory. I also suspect that if two test files ever share the same name, we may get a run-time error due to how install manifests work.

This is probably OK for now (assuming it does the right thing) though.
Attachment #8663603 - Flags: review?(gps) → review+
https://reviewboard.mozilla.org/r/19679/#review20171

> I'm pretty sure the shared "." here means that tests will all get copied to the same destination directory. I also suspect that if two test files ever share the same name, we may get a run-time error due to how install manifests work.
> 
> This is probably OK for now (assuming it does the right thing) though.

Yes, this can potentially break (like everything else can change) if the current directory structure changes.  Because of the way Marionette tests are currently structured, however, this should be fine and will give the most sensible hierarchy for packaging.
https://reviewboard.mozilla.org/r/19679/#review20171

> Yes, this can potentially break (like everything else can change) if the current directory structure changes.  Because of the way Marionette tests are currently structured, however, this should be fine and will give the most sensible hierarchy for packaging.

This is going to have the effect of symlinking all the tests to a destination in $objdir/_tests/marionette/ as though each test lived next to its manifest (so we'll have stuff like $objdir/_tests/marionette/testing/marionette/client/marionette/tests/update_test_status.py -> $srcdir/toolkit/mozapps/update/tests/marionette/update_test_status.py), which is ok, but this isn't the code that actually puts things in the test package (marionette has some one-off code for that in testsuite-targets.mk.

There are a couple of ways we could go with this. One would be to get rid of the one-off packaging stuff in testsuite-targets.mk and package things from $objdir/_tests, which would be good. Another would be to pass "False" as the last item here, which will not create symlinks, but will still make these tests available to |./mach test|. The current end result seems a bit confusing, and does have this side effect of requiring that no two marionette tests in the tree have the same leaf file name.
Comment on attachment 8663603 [details]
MozReview Request: Bug 1205687 - Package and include Marionette tests in all-tests.json; r?chmanchester r?gps

Bug 1205687: Package and include Marionette tests in all-tests.json

Listing the tests in all-tests.json is prerequisite to adding support
for Marionette tests to `./mach test FILE`.

r=chmanchester
r=gps
Comment on attachment 8663604 [details]
MozReview Request: Bug 1205687 - Add Mn test flavour and include tests in build manifests; r?jgraham

Bug 1205687: Add Mn test flavour and include tests in build manifests

r=jgraham
Comment on attachment 8663605 [details]
MozReview Request: Bug 1205687 - Add metadata about functional Loop tests; r?standard8

Bug 1205687: Add metadata about functional Loop tests

r=Standard8
Comment on attachment 8663606 [details]
MozReview Request: Bug 1205687 - Add metadata about Marionette layout tests; r?ms2ger

Bug 1205687: Add metadata about Marionette layout tests

r=Ms2ger
Comment on attachment 8663607 [details]
MozReview Request: Bug 1205687 - Add metadata about Marionette WebAPI tests; r?edgar r?shawnjohnjr

Bug 1205687: Add metadata about Marionette WebAPI tests

This patch also renames certain test files to avoid
duplication of test names.  Because they are all included in
testing/marionette/client/marionette/tests/webapi-tests.ini through
include statements, the test names must be unique.

r=echen
Attachment #8663607 - Flags: review+ → review?(shuang)
https://reviewboard.mozilla.org/r/19679/#review20171

> This is going to have the effect of symlinking all the tests to a destination in $objdir/_tests/marionette/ as though each test lived next to its manifest (so we'll have stuff like $objdir/_tests/marionette/testing/marionette/client/marionette/tests/update_test_status.py -> $srcdir/toolkit/mozapps/update/tests/marionette/update_test_status.py), which is ok, but this isn't the code that actually puts things in the test package (marionette has some one-off code for that in testsuite-targets.mk.
> 
> There are a couple of ways we could go with this. One would be to get rid of the one-off packaging stuff in testsuite-targets.mk and package things from $objdir/_tests, which would be good. Another would be to pass "False" as the last item here, which will not create symlinks, but will still make these tests available to |./mach test|. The current end result seems a bit confusing, and does have this side effect of requiring that no two marionette tests in the tree have the same leaf file name.

I agree that looks weird.  The goal here is to get `mach test FILE` working for Marionette, not to address the Marionette test packaging problems, so I’ve followed your advice and s/True/False/g in the above list.
Attachment #8663603 - Flags: review?(cmanchester)
Comment on attachment 8663603 [details]
MozReview Request: Bug 1205687 - Package and include Marionette tests in all-tests.json; r?chmanchester r?gps

Bug 1205687: Package and include Marionette tests in all-tests.json

Listing the tests in all-tests.json is prerequisite to adding support
for Marionette tests to `./mach test FILE`.

r=chmanchester
r=gps
Attachment #8663607 - Flags: review?(shuang) → review+
Attachment #8663607 - Flags: review+ → review?(shuang)
Comment on attachment 8663607 [details]
MozReview Request: Bug 1205687 - Add metadata about Marionette WebAPI tests; r?edgar r?shawnjohnjr

Bug 1205687: Add metadata about Marionette WebAPI tests

This patch also renames certain test files to avoid
duplication of test names.  Because they are all included in
testing/marionette/client/marionette/tests/webapi-tests.ini through
include statements, the test names must be unique.

r=echen
r=shawnjohnjr
Comment on attachment 8663603 [details]
MozReview Request: Bug 1205687 - Package and include Marionette tests in all-tests.json; r?chmanchester r?gps

https://reviewboard.mozilla.org/r/19679/#review20509

::: python/mozbuild/mozbuild/testing.py:180
(Diff revision 2)
> +            'marionette': os.path.join(self.topobjdir, '_tests', 'marionette'),

We can leave this out now that the tests aren't in that location.

::: python/mozbuild/mozbuild/testing.py:264
(Diff revision 2)
> +    MARIONETTE=('marionette', 'marionette', '.', False),
> +    MARIONETTE_LOOP=('marionette', 'marionette', '.', False),
> +    MARIONETTE_UNIT=('marionette', 'marionette', '.', False),
> +    MARIONETTE_UPDATE=('marionette', 'marionette', '.', False),
> +    MARIONETTE_WEBAPI=('marionette', 'marionette', '.', False),

Please add a short comment that we're running these tests from the srcdir and a todo about making packaging work like it does for the other suites.
Attachment #8663603 - Flags: review?(cmanchester) → review+
Comment on attachment 8663607 [details]
MozReview Request: Bug 1205687 - Add metadata about Marionette WebAPI tests; r?edgar r?shawnjohnjr

I think there is an another patch fix the naming problem in dom/bluetooth/tests/marionette/manifest.ini. So the patch is not necessary for dom/bluetooth/tests/marionette/manifest.ini.
Attachment #8663607 - Flags: review?(shuang) → review-
(In reply to Shawn Huang [:shawnjohnjr] from comment #31)
> I think there is an another patch fix the naming problem in
> dom/bluetooth/tests/marionette/manifest.ini. So the patch is not necessary
> for dom/bluetooth/tests/marionette/manifest.ini.

I noticed that a previous patch had fixed the file name issues during rebase.  The only change to this file now, after rebase, is fixing the EOF endline:

  https://reviewboard.mozilla.org/r/19687/diff/3#0

Is this change necessary to r-?  I can remove it from the changeset, but it would really be more work for me to do so.
Comment on attachment 8663603 [details]
MozReview Request: Bug 1205687 - Package and include Marionette tests in all-tests.json; r?chmanchester r?gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/19679/diff/2-3/
Attachment #8663603 - Attachment description: MozReview Request: Bug 1205687: Package and include Marionette tests in all-tests.json → MozReview Request: Bug 1205687 - Package and include Marionette tests in all-tests.json; r?chmanchester r?gps
Attachment #8663604 - Attachment description: MozReview Request: Bug 1205687: Add Mn test flavour and include tests in build manifests → MozReview Request: Bug 1205687 - Add Mn test flavour and include tests in build manifests; r?jgraham
Comment on attachment 8663604 [details]
MozReview Request: Bug 1205687 - Add Mn test flavour and include tests in build manifests; r?jgraham

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/19681/diff/2-3/
Comment on attachment 8663605 [details]
MozReview Request: Bug 1205687 - Add metadata about functional Loop tests; r?standard8

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/19683/diff/2-3/
Attachment #8663605 - Attachment description: MozReview Request: Bug 1205687: Add metadata about functional Loop tests → MozReview Request: Bug 1205687 - Add metadata about functional Loop tests; r?standard8
Comment on attachment 8663606 [details]
MozReview Request: Bug 1205687 - Add metadata about Marionette layout tests; r?ms2ger

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/19685/diff/2-3/
Attachment #8663606 - Attachment description: MozReview Request: Bug 1205687: Add metadata about Marionette layout tests → MozReview Request: Bug 1205687 - Add metadata about Marionette layout tests; r?ms2ger
Comment on attachment 8663607 [details]
MozReview Request: Bug 1205687 - Add metadata about Marionette WebAPI tests; r?edgar r?shawnjohnjr

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/19687/diff/3-4/
Attachment #8663607 - Attachment description: MozReview Request: Bug 1205687: Add metadata about Marionette WebAPI tests → MozReview Request: Bug 1205687 - Add metadata about Marionette WebAPI tests; r?echen r?shawnjohnjr
Attachment #8663607 - Flags: review- → review?(shuang)
Comment on attachment 8663607 [details]
MozReview Request: Bug 1205687 - Add metadata about Marionette WebAPI tests; r?edgar r?shawnjohnjr

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/19687/diff/4-5/
Attachment #8663607 - Attachment description: MozReview Request: Bug 1205687 - Add metadata about Marionette WebAPI tests; r?echen r?shawnjohnjr → MozReview Request: Bug 1205687 - Add metadata about Marionette WebAPI tests; r?edgar r?shawnjohnjr
(In reply to Andreas Tolfsen (:ato) from comment #38)
> Comment on attachment 8663607 [details]
> MozReview Request: Bug 1205687 - Add metadata about Marionette WebAPI tests;
> r?edgar r?shawnjohnjr
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/19687/diff/4-5/

Is this review request related to Bluetooth? I don't see there is anything related to Bluetooth.
Flags: needinfo?(ato)
(In reply to Shawn Huang [:shawnjohnjr] from comment #39)
> Is this review request related to Bluetooth? I don't see there is anything
> related to Bluetooth.

I can see no bluetooth related changes in the review anymore, but you r-’ed my previous review for removing a newline.
Flags: needinfo?(ato)
Comment on attachment 8663603 [details]
MozReview Request: Bug 1205687 - Package and include Marionette tests in all-tests.json; r?chmanchester r?gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/19679/diff/3-4/
Comment on attachment 8663604 [details]
MozReview Request: Bug 1205687 - Add Mn test flavour and include tests in build manifests; r?jgraham

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/19681/diff/3-4/
Comment on attachment 8663605 [details]
MozReview Request: Bug 1205687 - Add metadata about functional Loop tests; r?standard8

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/19683/diff/3-4/
Comment on attachment 8663606 [details]
MozReview Request: Bug 1205687 - Add metadata about Marionette layout tests; r?ms2ger

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/19685/diff/3-4/
Comment on attachment 8663607 [details]
MozReview Request: Bug 1205687 - Add metadata about Marionette WebAPI tests; r?edgar r?shawnjohnjr

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/19687/diff/5-6/
Attachment #8663607 - Flags: review+ → review?(shuang)
Comment on attachment 8663607 [details]
MozReview Request: Bug 1205687 - Add metadata about Marionette WebAPI tests; r?edgar r?shawnjohnjr

Carrying forward r+.
Attachment #8663607 - Flags: review?(shuang) → review+
Comment on attachment 8663603 [details]
MozReview Request: Bug 1205687 - Package and include Marionette tests in all-tests.json; r?chmanchester r?gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/19679/diff/4-5/
Comment on attachment 8663604 [details]
MozReview Request: Bug 1205687 - Add Mn test flavour and include tests in build manifests; r?jgraham

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/19681/diff/4-5/
Comment on attachment 8663605 [details]
MozReview Request: Bug 1205687 - Add metadata about functional Loop tests; r?standard8

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/19683/diff/4-5/
Comment on attachment 8663606 [details]
MozReview Request: Bug 1205687 - Add metadata about Marionette layout tests; r?ms2ger

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/19685/diff/4-5/
Comment on attachment 8663607 [details]
MozReview Request: Bug 1205687 - Add metadata about Marionette WebAPI tests; r?edgar r?shawnjohnjr

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/19687/diff/6-7/
Attachment #8663607 - Flags: review+ → review?(shuang)
Attachment #8663607 - Flags: review?(shuang) → review+
You need to log in before you can comment on or make changes to this bug.