Closed Bug 1368264 Opened 7 years ago Closed 7 years ago

Hook testing/geckodriver up to WPT

Categories

(Remote Protocol :: Marionette, enhancement, P1)

Version 3
enhancement

Tracking

(firefox55 fixed)

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: ato, Assigned: ato)

References

Details

Attachments

(4 files)

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: nobody → ato
Status: NEW → ASSIGNED
Depends on: 1368035
Priority: -- → P1
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
Attachment #8873557 - Flags: review?(nfroyd)
Attachment #8873556 - Flags: review?(james)
Attachment #8873558 - Flags: review?(james)
Attachment #8874389 - Flags: review?(james)
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 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 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 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 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+
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.
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.
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.
Blocks: 1370636
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 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.
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
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: