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

Add action_sequence in testdriver #13640

Merged
merged 2 commits into from
Nov 13, 2018

Conversation

chromium-wpt-export-bot
Copy link
Collaborator

@chromium-wpt-export-bot chromium-wpt-export-bot commented Oct 21, 2018

After we expose test_driver.Actions API to web users, we add their
implementation in our testdriver file, and fix the wpt tests of
Actions API.

Bug: 893480
Change-Id: Ib02c0223208eeb2cc30c2ca35b98d5fc938baa2c
Reviewed-on: https://chromium-review.googlesource.com/c/1289119
Commit-Queue: Lan Wei <lanwei@chromium.org>
Reviewed-by: Navid Zolghadr <nzolghadr@chromium.org>
Reviewed-by: Dave Tapuska <dtapuska@chromium.org>
Cr-Commit-Position: refs/heads/master@{#606882}

Copy link
Collaborator

@wpt-pr-bot wpt-pr-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already reviewed downstream.

@chromium-wpt-export-bot chromium-wpt-export-bot force-pushed the chromium-export-cl-1289119 branch 2 times, most recently from 0626563 to d726acb Compare October 23, 2018 19:21
@chromium-wpt-export-bot chromium-wpt-export-bot changed the title Add action_sequence in testdriver Modify the web platform tests with pen inputs to use TestDriver Nov 6, 2018
@Hexcles Hexcles changed the title Modify the web platform tests with pen inputs to use TestDriver Add action_sequence in testdriver Nov 6, 2018
@foolip foolip mentioned this pull request Nov 7, 2018
@chromium-wpt-export-bot chromium-wpt-export-bot force-pushed the chromium-export-cl-1289119 branch 6 times, most recently from 13e26a4 to ce41218 Compare November 9, 2018 17:32
@foolip
Copy link
Member

foolip commented Nov 13, 2018

@Summerlw @NavidZ, this change has gotten stuck and not exported properly because of failing infrastructure/ tests. These aren't run in Chromium in precisely the same way as here, so that's unfortunately a possibility.

Are either of you able to take a look at the failures here and suggest a fix?

LanWei22 and others added 2 commits November 13, 2018 11:20
After we expose test_driver.Actions API to web users, we add their
implementation in our testdriver file, and fix the wpt tests of
Actions API.

Bug: 893480
Change-Id: Ib02c0223208eeb2cc30c2ca35b98d5fc938baa2c
Reviewed-on: https://chromium-review.googlesource.com/c/1289119
Commit-Queue: Lan Wei <lanwei@chromium.org>
Reviewed-by: Navid Zolghadr <nzolghadr@chromium.org>
Reviewed-by: Dave Tapuska <dtapuska@chromium.org>
Cr-Commit-Position: refs/heads/master@{#606882}
@foolip foolip force-pushed the chromium-export-cl-1289119 branch from ce41218 to c2bf7c6 Compare November 13, 2018 10:31
@foolip
Copy link
Member

foolip commented Nov 13, 2018

I've pushed a commit to update the expectations. That might allow this PR to land, but since the tests are still failing I presume there's more work to be done here anyway :)

@chromium-wpt-export-bot chromium-wpt-export-bot merged commit 683ea00 into master Nov 13, 2018
@chromium-wpt-export-bot chromium-wpt-export-bot deleted the chromium-export-cl-1289119 branch November 13, 2018 10:41
@NavidZ
Copy link
Member

NavidZ commented Nov 13, 2018

I believe this has failed due to the pointerdown now getting the coordinates. w3c/webdriver#1348 is addressing the issue. I'm working on sending a pull request to webdriver spec as well.
@jgraham FYI.

@jgraham
Copy link
Contributor

jgraham commented Nov 15, 2018

Yeah, this should never have landed. Having to update the metadata for infrastructure tests to expected: FAIL everywhere should be a pretty clear indication that there's something broken in the changeset (in this case that they're assuming an API that just isn't like the one that currently exists; that difference is indeed a missing piece of flexibility in the current API, but it's still not OK to just change tests to assume something totally nonstandard, even if the standard is lacking).

@gsnedders gsnedders mentioned this pull request Nov 15, 2018
@foolip
Copy link
Member

foolip commented Nov 15, 2018

Sorry, I moved too fast here. The fact that the infrastructure tests were previously passing in Firefox wasn't apparent to me, I just treated it as "broken before, still broken" and needing some follow-up work.

What is the non-standard part here, though? I don't see any changes to the WebDriver layer, where is it hiding?

@foolip
Copy link
Member

foolip commented Nov 15, 2018

@lukebjerring yet another case where regression detection would have saved the day :)

@NavidZ
Copy link
Member

NavidZ commented Nov 15, 2018

Yeah. Now the changes are more hidden. Any changes to the serialized json coming out of send function of testdriver could break this. Basically we added a coordinate pair to the pointerdown action as well which we thought we needed to address some of the missing use cases. Then this get encoded in that json later.
As we discussed in w3c/webdriver#1348 we are going to instead add another flag to each command to suppress sending the command (and only let the driver update the state of input including coordinate and button state).
For now we leave those tests that need something like this as manual and essentially ignore those test and only migrate the tests that can work with the existing functionalities.

@foolip
Copy link
Member

foolip commented Nov 15, 2018

OK, so some of this code changed the JSON payload for the WebDriver commands. Then I understand how it could be non-standard.

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

Successfully merging this pull request may close these issues.

6 participants