Hook testing/geckodriver up to WPT

RESOLVED FIXED in Firefox 55

Status

Testing
Marionette
P1
normal
RESOLVED FIXED
9 months ago
9 months ago

People

(Reporter: ato, Assigned: ato)

Tracking

(Blocks: 1 bug)

Version 3
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments)

(Assignee)

Description

9 months ago
When ENABLE_GECKODRIVER (introduced in https://bugzilla.mozilla.org/show_bug.cgi?id=1368035), we should copy the geckodriver binary to dist/bin so it can be included with the test package we ship to test slaves.
(Assignee)

Updated

9 months ago
Assignee: nobody → ato
Status: NEW → ASSIGNED
Depends on: 1368035
Priority: -- → P1
(Assignee)

Comment 1

9 months ago
We also want to hook the geckodriver binary produces on the build slaves to the WPT wdspec tests.
Summary: Copy geckodriver binary to dist/bin → Hook testing/geckodriver up to WPT
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Blocks: 1364389
Blocks: 1370223
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

9 months ago
Attachment #8873557 - Flags: review?(nfroyd)
Attachment #8873556 - Flags: review?(james)
Attachment #8873558 - Flags: review?(james)
Attachment #8874389 - Flags: review?(james)
(Assignee)

Comment 11

9 months ago
Latest try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cfbbe0d6f32d

The Wr job on Win x64 [sic] has since been corrected on m-c, but this branch doesn’t have that commit.  The intermittents on Wd for Linux x64 Debug are known.

Comment 12

9 months ago
mozreview-review
Comment on attachment 8873557 [details]
Bug 1368264 - Fix missing RUST_PROGRAMS entry in if-condition

https://reviewboard.mozilla.org/r/144942/#review150188

Is geckodriver really a target program?  Is that really correct for Android?
Attachment #8873557 - Flags: review?(nfroyd) → review+

Comment 13

9 months ago
mozreview-review
Comment on attachment 8874389 [details]
Bug 1368264 - Make WPT use WebDriver binary from test archive

https://reviewboard.mozilla.org/r/145752/#review150194

::: testing/mozharness/scripts/web_platform_tests.py:177
(Diff revision 3)
>          if "wdspec" in c.get("test_type", []):
> -            assert self.geckodriver_path is not None
> -            cmd.append("--webdriver-binary=%s" % self.geckodriver_path)
> +            geckodriver_path = os.path.join(dirs["abs_test_bin_dir"], "geckodriver")
> +            if not os.path.isfile(geckodriver_path):
> +                self.fatal("Unable to find geckodriver binary "
> +                           "in common test package: %s" % geckodriver_path)
> +            cmd.append("--webdriver-binary=%s" % geckodriver_path)

I guess this will fail on Windows, but OK to fix that later.
Attachment #8874389 - Flags: review?(james) → review+

Comment 14

9 months ago
mozreview-review
Comment on attachment 8873556 [details]
Bug 1368264 - Have mach pick up geckodriver from dist/bin

https://reviewboard.mozilla.org/r/144940/#review150196
Attachment #8873556 - Flags: review?(james) → review+

Comment 15

9 months ago
mozreview-review
Comment on attachment 8873558 [details]
Bug 1368264 - Include geckodriver in test archive

https://reviewboard.mozilla.org/r/144944/#review150200

::: python/mozbuild/mozbuild/action/test_archive.py:40
(Diff revision 2)
> -    'SmokeDMD',
>      'certutil',
>      'crashinject',
>      'fileid',
> +    'geckodriver',
> +    'GenerateOCSPResponse',

Does this work? I expect it to complain that it's not in unicode codepoint order anymore. I suggest reverting these changes.
Attachment #8873558 - Flags: review?(james) → review+
(Assignee)

Comment 16

9 months ago
mozreview-review-reply
Comment on attachment 8873558 [details]
Bug 1368264 - Include geckodriver in test archive

https://reviewboard.mozilla.org/r/144944/#review150200

> Does this work? I expect it to complain that it's not in unicode codepoint order anymore. I suggest reverting these changes.

I just ran sort(1) over the list to put geckodriver in the right place, which I guess reordered it in lexicographical order.  I don’t see any regressions from this change, but if you think it’s important I can edit the patch again.
(Assignee)

Comment 17

9 months ago
mozreview-review-reply
Comment on attachment 8873557 [details]
Bug 1368264 - Fix missing RUST_PROGRAMS entry in if-condition

https://reviewboard.mozilla.org/r/144942/#review150188

It would be in the future.  We have disabled compilation of geckodriver for Android at the moment because we need to figure out how to cross-compile it for the target system instead of the host.  When this is fixed, it will also for this platform emit a geckodriver binary that we will want to move to dist/bin.
(Assignee)

Comment 18

9 months ago
mozreview-review-reply
Comment on attachment 8874389 [details]
Bug 1368264 - Make WPT use WebDriver binary from test archive

https://reviewboard.mozilla.org/r/145752/#review150194

> I guess this will fail on Windows, but OK to fix that later.

Good point.  I’ve filed https://bugzilla.mozilla.org/show_bug.cgi?id=1370636 to enable the Wd job on Windows platforms.  I will address this there.
(Assignee)

Updated

9 months ago
Blocks: 1370636

Comment 19

9 months ago
mozreview-review-reply
Comment on attachment 8873558 [details]
Bug 1368264 - Include geckodriver in test archive

https://reviewboard.mozilla.org/r/144944/#review150200

> I just ran sort(1) over the list to put geckodriver in the right place, which I guess reordered it in lexicographical order.  I don’t see any regressions from this change, but if you think it’s important I can edit the patch again.

I'm moderately surprised the build jobs aren't failing, and I think that I would prefer this change is reversed; sorting by codepoint order is a legitimate sort method and in particular is the way Python sorts by default (which is therefore what I expect this to use).
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 24

9 months ago
mozreview-review-reply
Comment on attachment 8873558 [details]
Bug 1368264 - Include geckodriver in test archive

https://reviewboard.mozilla.org/r/144944/#review150200

> I'm moderately surprised the build jobs aren't failing, and I think that I would prefer this change is reversed; sorting by codepoint order is a legitimate sort method and in particular is the way Python sorts by default (which is therefore what I expect this to use).

OK, thanks for the background.

Comment 25

9 months ago
Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9eec39879b33
Fix missing RUST_PROGRAMS entry in if-condition r=froydnj
https://hg.mozilla.org/integration/autoland/rev/332dae5c381b
Have mach pick up geckodriver from dist/bin r=jgraham
https://hg.mozilla.org/integration/autoland/rev/3f7e348f0ff2
Include geckodriver in test archive r=jgraham
https://hg.mozilla.org/integration/autoland/rev/447882512ee8
Make WPT use WebDriver binary from test archive r=jgraham

Comment 26

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9eec39879b33
https://hg.mozilla.org/mozilla-central/rev/332dae5c381b
https://hg.mozilla.org/mozilla-central/rev/3f7e348f0ff2
https://hg.mozilla.org/mozilla-central/rev/447882512ee8
Status: ASSIGNED → RESOLVED
Last Resolved: 9 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.