Skip to content
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 #20437

Closed
jdm opened this issue Mar 26, 2018 · 8 comments
Closed

Enable upstream bluetooth tests #20437

jdm opened this issue Mar 26, 2018 · 8 comments

Comments

@jdm
Copy link
Member

@jdm jdm commented Mar 26, 2018

There are now tests being shared between browsers available at tests/wpt/web-platform-tests/bluetooth. We should enable these tests and figure out what's missing in our testing setup in order to run them correctly. At minimum we'll need an ini file like this one, and we should compare the results we get against the existing results in this directory.

@cdeler
Copy link
Contributor

@cdeler cdeler commented Dec 10, 2018

Hello again @jdm ,
What should I do to start working on this issue? Should I notify someone except you?

@jdm
Copy link
Member Author

@jdm jdm commented Dec 10, 2018

@cdeler It requires enabling the test directory, then running ./mach test-wpt tests/wpt/web-platform-tests/bluetooth --log-raw log, followed by ./Mach update-wpt log. This will create a bunch of new ini files for the tests that don't pass, so the next step is to compare those result against the ini files for the existing tests and see if there are any significant differences.

@cdeler
Copy link
Contributor

@cdeler cdeler commented Dec 10, 2018

@jdm
Well, I still have a little question.

The most of tests didn't execute (were skipped), as long as I've made a little fix in

(SKV) user ~/workspace/skv/servo % git diff tests/wpt/web-platform-tests/tools/wptrunner/wptrunner/executors/executorservo.py
diff --git a/tests/wpt/web-platform-tests/tools/wptrunner/wptrunner/executors/executorservo.py b/tests/wpt/web-platform-tests/tools/wptrunner/wptrunner/executors/executorservo.py
index 784356bb8b..1e1a4644d0 100644
--- a/tests/wpt/web-platform-tests/tools/wptrunner/wptrunner/executors/executorservo.py
+++ b/tests/wpt/web-platform-tests/tools/wptrunner/wptrunner/executors/executorservo.py
@@ -33,6 +33,7 @@ def write_hosts_file(config):
 
 
 class ServoTestharnessExecutor(ProcessTestExecutor):
+    supports_testdriver = True
     convert_result = testharness_result_converter
 
     def __init__(self, browser, server_config, timeout_multiplier=1, debug_info=None,

The reason of skipping was this condition:
https://github.com/servo/servo/blob/master/tests/wpt/web-platform-tests/tools/wptrunner/wptrunner/wptrunner.py#L248
test.testdriver and not executor_cls.supports_testdriver was True (and executor_cls is ServoTestharnessExecutor)

Therefore I added (locally) supports_testdriver = True to the ServoTestharnessExecutor class.

Since that all bluetooth tests are executed without skipping (but most of them are failed).

As I'm still not much familiar with this tests, I want to ask you, is this change normally, or I haven't understood something?

PS I also added the bluetooth section to the tests/wpt/include.ini

@cdeler
Copy link
Contributor

@cdeler cdeler commented Dec 10, 2018

@jdm
I executed the tests like
./mach test-wpt tests/wpt/web-platform-tests/bluetooth/ --headless --log-raw log3.out

@jdm
Copy link
Member Author

@jdm jdm commented Dec 11, 2018

Very interesting results; I have now done some reading about what testdriver is and it's a little more clear to me what the next step is. I suspect we will want to build on top of #22411 and run the tests with --product=servodriver, and add the supports_testdriver = True line to executorservodriver.py instead.

This should get us to a baseline where we can evaluate the state of the tests (for example, I filed #22412 based on some failures I observed when running some tests, but other ones passed). Rather than modifying include.ini to always run those tests, I think we will need to include a special CI step in etc/ci/buildbot-steps.yml (like I did in #22411) that only runs the bluetooth directory. This will allow us to add proper support for for the testdriver API over time as we figure out what is required.

So, what I would like to see is a PR that adds a CI step to run the new tests using the servodriver product, along with new test results based on running ./mach update-wpt log where log is generated from a local run of the tests using servodriver. Does that make sense?

@cdeler
Copy link
Contributor

@cdeler cdeler commented Dec 11, 2018

Does that make sense?

Yes, I've got it, thank you

cdeler added a commit to cdeler/servo that referenced this issue Dec 11, 2018
cdeler added a commit to cdeler/servo that referenced this issue Dec 11, 2018
cdeler added a commit to cdeler/servo that referenced this issue Dec 11, 2018
cdeler added a commit to cdeler/servo that referenced this issue Dec 11, 2018
@cdeler cdeler mentioned this issue Dec 11, 2018
4 of 5 tasks complete
@cdeler
Copy link
Contributor

@cdeler cdeler commented Dec 12, 2018

@jdm
Is it possible to ask you review the PR #22420 ?

@jdm
Copy link
Member Author

@jdm jdm commented Dec 12, 2018

Yes. I will review it today.

cdeler added a commit to cdeler/servo that referenced this issue Dec 14, 2018
cdeler added a commit to cdeler/servo that referenced this issue Dec 15, 2018
cdeler added a commit to cdeler/servo that referenced this issue Dec 15, 2018
cdeler added a commit to cdeler/servo that referenced this issue Dec 15, 2018
cdeler added a commit to cdeler/servo that referenced this issue Dec 15, 2018
cdeler added a commit to cdeler/servo that referenced this issue Dec 15, 2018
cdeler added a commit to cdeler/servo that referenced this issue Dec 15, 2018
cdeler added a commit to cdeler/servo that referenced this issue Dec 15, 2018
cdeler added a commit to cdeler/servo that referenced this issue Dec 16, 2018
cdeler added a commit to cdeler/servo that referenced this issue Dec 16, 2018
cdeler added a commit to cdeler/servo that referenced this issue Dec 16, 2018
cdeler added a commit to cdeler/servo that referenced this issue Dec 16, 2018
cdeler added a commit to cdeler/servo that referenced this issue Dec 16, 2018
cdeler added a commit to cdeler/servo that referenced this issue Dec 16, 2018
cdeler added a commit to cdeler/servo that referenced this issue Dec 16, 2018
bors-servo added a commit that referenced this issue Dec 17, 2018
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 added a commit that referenced this issue Dec 22, 2018
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 -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

2 participants
You can’t perform that action at this time.