New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable upstream bluetooth tests #22420

Merged
merged 9 commits into from Dec 22, 2018

Conversation

Projects
None yet
6 participants
@cdeler
Copy link
Contributor

cdeler commented Dec 11, 2018

I enabled bluetooth WPT tests

But I'm still worry that a lot of tests failed (more than 150). I decreased them by

prefs: ["dom.bluetooth.enabled:true"]

But it's still a lot of failed tests

Checks

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #20437 (GitHub issue number if applicable)
  • There are tests for these changes OR
  • These changes do not require tests because the changes are tests

This change is Reviewable

@highfive

This comment has been minimized.

Copy link

highfive commented Dec 11, 2018

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @avadacatavra (or someone else) soon.

@highfive

This comment has been minimized.

Copy link

highfive commented Dec 11, 2018

Heads up! This PR modifies the following files:

@servo-wpt-sync

This comment has been minimized.

Copy link
Collaborator

servo-wpt-sync commented Dec 11, 2018

Opened new PR for upstreamable changes.

Completed upstream sync of web-platform-test changes at web-platform-tests/wpt#14468.

@jdm

This comment has been minimized.

Copy link
Member

jdm commented Dec 14, 2018

r? @jdm

@highfive highfive assigned jdm and unassigned avadacatavra Dec 14, 2018

@jdm
Copy link
Member

jdm left a comment

Currently the tests are running on the non-webdriver test runner, but if we actually want to support the testdriver.js api one day (since the tests require it), we will need to use the webdriver-enabled test runner (servodriver) instead.

@@ -5,6 +5,8 @@ skip: true
skip: false
[2dcontext]
skip: false
[bluetooth]
skip: false

This comment has been minimized.

@jdm

jdm Dec 14, 2018

Member

Let's remove this. The test expectations won't match when run in the non-servodriver test runs.

This comment has been minimized.

@cdeler

cdeler Dec 14, 2018

Contributor

Done

@@ -33,6 +33,7 @@ def write_hosts_file(config):


class ServoTestharnessExecutor(ProcessTestExecutor):
supports_testdriver = True

This comment has been minimized.

@jdm

jdm Dec 14, 2018

Member

Let's move this to executorservodriver.py instead.

This comment has been minimized.

@jdm

jdm Dec 14, 2018

Member

This has not been addressed.

This comment has been minimized.

@cdeler

cdeler Dec 15, 2018

Contributor

Done

@@ -63,6 +63,14 @@ mac-rel-css2:
- ./mach test-wpt --release --processes 4 --total-chunks 6 --this-chunk 6 --log-raw test-wpt.log --log-errorsummary wpt-errorsummary.log --always-succeed
- ./mach filter-intermittents wpt-errorsummary.log --log-intermittents intermittents.log --log-filteredsummary filtered-wpt-errorsummary.log --tracker-api default --reporter-api default

mac-rel-bluetooth:

This comment has been minimized.

@jdm

jdm Dec 14, 2018

Member

Instead of a different job, please add this to an existing mac job that runs WPT tests.

This comment has been minimized.

@cdeler

cdeler Dec 14, 2018

Contributor

Ok, I've moved the execution to mac-rel-wpt4

- ./mach clean-nightlies --keep 3 --force
- ./mach clean-cargo-cache --keep 3 --force
- env PKG_CONFIG_PATH=/usr/local/opt/zlib/lib/pkgconfig ./mach build --release
- ./mach test-wpt --release --processes=8 --log-raw test-bluetooth.log --log-errorsummary bluetooth-errorsummary.log --always-succeed bluetooth

This comment has been minimized.

@jdm

jdm Dec 14, 2018

Member

We will need to pass --product=servodriver with the other changes that I recommended.

This comment has been minimized.

@jdm

jdm Dec 14, 2018

Member

This still has not been addressed.

This comment has been minimized.

@cdeler

cdeler Dec 14, 2018

Contributor

Yes, thank you, Im sorry for delay

servo rebuild takes a while after my rebase to upstream; I just wanted to get #22411 in my repo

This comment has been minimized.

@cdeler

cdeler Dec 15, 2018

Contributor

It's done, thank you

I've also added --headless to the bluetooth tests execution

@servo-wpt-sync

This comment has been minimized.

Copy link
Collaborator

servo-wpt-sync commented Dec 14, 2018

Transplanted upstreamable changes to existing PR.

Completed upstream sync of web-platform-test changes at web-platform-tests/wpt#14468.

@cdeler cdeler force-pushed the cdeler:enable-upstream-bluetooth-tests branch from 91e9a03 to cb9b9b5 Dec 15, 2018

@servo-wpt-sync

This comment has been minimized.

Copy link
Collaborator

servo-wpt-sync commented Dec 15, 2018

Transplanted upstreamable changes to existing PR.

Completed upstream sync of web-platform-test changes at web-platform-tests/wpt#14468.

1 similar comment
@servo-wpt-sync

This comment has been minimized.

Copy link
Collaborator

servo-wpt-sync commented Dec 15, 2018

Transplanted upstreamable changes to existing PR.

Completed upstream sync of web-platform-test changes at web-platform-tests/wpt#14468.

@cdeler

This comment has been minimized.

Copy link
Contributor

cdeler commented Dec 15, 2018

@jdm I've fixed the requested changes,

But seems like one of CI builds failed.

I'm trying to figure out whats wrong with the broken test

@cdeler cdeler force-pushed the cdeler:enable-upstream-bluetooth-tests branch from 4022ba6 to 1b6a36b Dec 16, 2018

@servo-wpt-sync

This comment has been minimized.

Copy link
Collaborator

servo-wpt-sync commented Dec 16, 2018

Transplanted upstreamable changes to existing PR.

Completed upstream sync of web-platform-test changes at web-platform-tests/wpt#14468.

@jdm

This comment has been minimized.

Copy link
Member

jdm commented Dec 17, 2018

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 17, 2018

📌 Commit 1b6a36b has been approved by jdm

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 17, 2018

⌛️ Testing commit 1b6a36b with merge 7f64746...

bors-servo added a commit that referenced this pull request Dec 17, 2018

Auto merge of #22420 - cdeler:enable-upstream-bluetooth-tests, r=jdm
Enable upstream bluetooth tests

I enabled bluetooth WPT tests

But I'm still worry that a lot of tests failed (more than 150).  I decreased them by
```
prefs: ["dom.bluetooth.enabled:true"]
```
But it's still a lot of failed tests

Checks
---
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #20437 (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [X] These changes do not require tests because the changes are tests

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22420)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 17, 2018

💔 Test failed - status-taskcluster

@cdeler

This comment has been minimized.

Copy link
Contributor

cdeler commented Dec 17, 2018

💔 Test failed - status-taskcluster

I'm not sure that the bluetooth tests enabling could affect tests like:

/quirks/unitless-length/limited-quirks.html
/xhr/open-url-multi-window.htm

(newertheless supports_testdriver = True could)

But some of the tests like /custom-elements/parser/parser-sets-attributes-and-children.html are failing in other branches (e.g. cdeler/implement-DOMException-constructor or master)

@jdm

This comment has been minimized.

Copy link
Member

jdm commented Dec 17, 2018

Yeah, our taskcluster test runner is having big problems right now. This failure isn't caused by these changes.

@cdeler

This comment has been minimized.

Copy link
Contributor

cdeler commented Dec 19, 2018

@jdm Hello,
Do you know, what should I do to help merging the PR?

@jdm

This comment has been minimized.

Copy link
Member

jdm commented Dec 19, 2018

There is nothing you can or should do. #22493 is my attempt to improve the situation.

@jdm

This comment has been minimized.

Copy link
Member

jdm commented Dec 22, 2018

@bors-servo retry

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 22, 2018

⌛️ Testing commit 1b6a36b with merge 12d0a0b...

bors-servo added a commit that referenced this pull request Dec 22, 2018

Auto merge of #22420 - cdeler:enable-upstream-bluetooth-tests, r=jdm
Enable upstream bluetooth tests

I enabled bluetooth WPT tests

But I'm still worry that a lot of tests failed (more than 150).  I decreased them by
```
prefs: ["dom.bluetooth.enabled:true"]
```
But it's still a lot of failed tests

Checks
---
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #20437 (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [X] These changes do not require tests because the changes are tests

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22420)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 22, 2018

@bors-servo bors-servo merged commit 1b6a36b into servo:master Dec 22, 2018

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@servo-wpt-sync

This comment has been minimized.

Copy link
Collaborator

servo-wpt-sync commented Dec 22, 2018

Error syncing changes upstream. Logs saved in error-snapshot-1545470803628.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment