Added constructor tests for Touch and TouchEvent #2299

chong-z · 2015-11-03T15:27:22Z

@RByers Added constructor tests for Touch and TouchEvent based on Touch Events v2 - Touch Interface and Touch Events v2 - TouchEvent Interface

Browser support:
Chrome Canary: Yes
Firefox, Safari, Edge: Not yet

Issue tracking: w3c/touch-events#27

hoppipolla-critic-bot · 2015-11-03T15:27:26Z

Critic review: https://critic.hoppipolla.co.uk/r/5930

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.

RByers · 2015-11-03T18:06:31Z

Thanks for doing this! Just a couple small concerns.

I've actually never contributed directly to web-platform-test before (I worked on these tests when they were hosted directly in the webevents repository), so I'm not completely sure what the process is. Looks like there's some information here.

Please send a note to public-touchevents mentioning this pull request (pull requests to the touchevents repo automatically generate a message to the list, but this is different) so that anyone else in the group can take a look.

I think we should land the feature in blink and verify these tests pass in Chrome Canary before landing the new tests.

RByers · 2015-11-03T18:09:32Z

Please send a note to public-touchevents mentioning this pull request (pull requests to the touchevents repo automatically generate a message to the list, but this is different) so that anyone else in the group can take a look.

Actually my comment on the issue should be sufficient (looks like all issue comments get copied to the list).

AFBarstow · 2015-11-25T12:51:25Z

This issue is also logged as w3c/touch-events#27

jgraham · 2015-12-23T08:43:07Z

To get these merged someone needs to review them. @RByers - are you doing that? Then, ideally, the commits would be squashed, then we can merge.

RByers · 2015-12-23T13:39:28Z

@jgraham Yes I reviewed them back in Nov, but I don't have much experience with WPT so I assumed we'd need review from someone else too. @choniong, you had talked to someone on #testing about getting a review right?

RByers · 2015-12-23T13:46:50Z

Review status: 0 of 3 files reviewed at latest revision, 1 unresolved discussion.

touch-events/touch-touchevent-constructor.html, line 15 [r3] (raw file):
Please update this comment. None of the previous 3 paragraphs applies to this test. Maybe instead just say something brief about testing the constructors.

Comments from the review on Reviewable.io

RByers · 2015-12-23T13:47:40Z

Reviewed 1 of 3 files at r2, 2 of 2 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

Comments from the review on Reviewable.io

Ms2ger · 2015-12-23T16:02:08Z

Review status: all files reviewed at latest revision, 9 unresolved discussions.

touch-events/create-touch-touchlist.html, line 36 [r3] (raw file):
Why did you add this? If createTouch doesn't exist, the following line will throw and cause a test failure.

touch-events/touch-touchevent-constructor.html, line 19 [r3] (raw file):
No need for this.

touch-events/touch-touchevent-constructor.html, line 28 [r3] (raw file):
There doesn't seem to be a reason to wait for the load event. Also remove the setup/done calls.

touch-events/touch-touchevent-constructor.html, line 30 [r3] (raw file):
Please move those vars to where they're used; they're not shared between tests (which would be bad practice).

touch-events/touch-touchevent-constructor.html, line 65 [r3] (raw file):
No need for the assignments.

touch-events/touch-touchevent-constructor.html, line 128 [r3] (raw file):
No need to do this.

touch-events/touch-touchevent-constructor.html, line 142 [r3] (raw file):
Remove the styling

touch-events/touch-touchevent-constructor.html, line 147 [r3] (raw file):
Remove the text; it serves no purpose.

Comments from the review on Reviewable.io

RByers · 2015-12-23T16:22:23Z

Review status: all files reviewed at latest revision, 9 unresolved discussions.

touch-events/touch-touchevent-constructor.html, line 19 [r3] (raw file):
Why do you say that? We want the tests to be easily usable on mobile and without this you need to do a lot of pinch-zooming to read the test results. Also note that this is already present in all the other touch event tests.

touch-events/touch-touchevent-constructor.html, line 28 [r3] (raw file):
Note that Chong was just copying the pattern used by the existing touch tests. I agree that it's worth cleaning up the test when adding a new one, but presumably at some point we should have someone go back and apply all this feedback to the existing tests this one is based on too.

Comments from the review on Reviewable.io

RByers · 2015-12-23T16:25:05Z

@Ms2ger thanks for doing a review, we really appreciate it! Big picture I'd like to get my blink input team (and ultimately the rest of Google web platform teams) contributing much more actively to WPT, so I appreciate your patience while we get ramped up on the process / style.

Ms2ger · 2015-12-23T16:56:02Z

Review status: all files reviewed at latest revision, 9 unresolved discussions.

touch-events/touch-touchevent-constructor.html, line 19 [r3] (raw file):
Looking on my phone, it doesn't seem to improve much? Anyway, we could discuss that outside this PR if you feel strongly.

touch-events/touch-touchevent-constructor.html, line 28 [r3] (raw file):
I may end up fixing the existing tests at some point, and I don't blame anyone for picking up sub-optimal habits, but I'd like to avoid them in new tests.

Comments from the review on Reviewable.io

chong-z · 2015-12-23T17:08:15Z

Reviewed 2 of 3 files at r2.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

touch-events/create-touch-touchlist.html, line 36 [r3] (raw file):
Because I assume
"assert_true: document.createTouch exists expected true got false"
has more readability than
"document.createTouch is not a function TypeError: document.createTouch is not a function"
But sure I can remove this.

touch-events/touch-touchevent-constructor.html, line 28 [r3] (raw file):
Sure I will try to fit them into the right style. Thanks for doing the review!

Comments from the review on Reviewable.io

RByers · 2015-12-23T17:29:07Z

Review status: all files reviewed at latest revision, 3 unresolved discussions.

touch-events/touch-touchevent-constructor.html, line 19 [r3] (raw file):
I don't feel particularly strongly on this detail, just that in general WPT tests (especially mobile-focused ones) should be designed to work well on mobile. How about we keep all the touch tests consistent in this regard for now and follow up in w3c/touch-events#56?

touch-events/touch-touchevent-constructor.html, line 28 [r3] (raw file):
@Ms2ger agreed, except you shouldn't have to update the tests yourself. We (TECG members) are happy to do that as long as you (or someone else) can provide guidance / review. Filed w3c/touch-events##57 to track.

Comments from the review on Reviewable.io

RByers · 2015-12-23T17:29:42Z

Review status: all files reviewed at latest revision, 3 unresolved discussions.

touch-events/touch-touchevent-constructor.html, line 28 [r3] (raw file):
Sorry, that's w3c/touch-events#57

Comments from the review on Reviewable.io

chong-z · 2016-02-18T20:02:30Z

Review status: 0 of 3 files reviewed at latest revision, 9 unresolved discussions.

touch-events/create-touch-touchlist.html, line 36 [r3] (raw file):
Done.

touch-events/touch-touchevent-constructor.html, line 15 [r3] (raw file):
Done.

touch-events/touch-touchevent-constructor.html, line 19 [r3] (raw file):
Done.

touch-events/touch-touchevent-constructor.html, line 28 [r3] (raw file):
Done.

touch-events/touch-touchevent-constructor.html, line 30 [r3] (raw file):
Done.

touch-events/touch-touchevent-constructor.html, line 65 [r3] (raw file):
Done.

touch-events/touch-touchevent-constructor.html, line 128 [r3] (raw file):
Done.

touch-events/touch-touchevent-constructor.html, line 142 [r3] (raw file):
Done.

touch-events/touch-touchevent-constructor.html, line 147 [r3] (raw file):
Done.

Comments from the review on Reviewable.io

chong-z · 2016-02-22T15:12:31Z

Hi @Ms2ger , sorry for the slow response, but can you take a look at the updated PR please? Thanks!

Ms2ger · 2016-02-25T10:46:46Z

touch-events/touch-touchevent-constructor.html

+<div id="target0"></div>
+<script>
+test(function() {
+    assert_throws({name: 'TypeError'}, function() {new Touch();}, "Touch constructor requires initialize dictionary");


assert_throws(new TypeError(), ...) is more idiomatic

Ms2ger · 2016-02-25T10:48:25Z

On the testharness side, this looks good to me with the one nit fixed. No comment on whether the tests are correct otherwise.

chong-z · 2016-02-25T17:12:50Z

Thanks for the prompt response! I've fixed the nit, so is it ok to merge or do I need to ask for another correctness review?

RByers · 2016-02-25T17:27:32Z

Thanks for the prompt response! I've fixed the nit, so is it ok to merge or do I need to ask for another correctness review?

I looked over the correctness again, and it still looks good to me. Please run your test on Chrome after your final changes (if you haven't already) to verify it still passes. Then I'm guessing @Ms2ger will want you to squash all your commits into a single one (use git rebase -i) before he merges.

Moved common test code into a shared JS Removed unused touchList code Switch to common touch-support.js Test with insufficient and minimum properties Fixed duplicate test name issue Fix style as per Ms2ger's comments Use 'new TypeError()' for assert_throws

chong-z · 2016-02-25T17:38:53Z

Sure I've tested on Chrome and it still passes, and commits are squashed into one now. Thanks!

Added constructor tests for Touch and TouchEvent

jdm · 2016-02-26T21:23:20Z

Thank you @choniong!

RByers mentioned this pull request Nov 3, 2015

Add tests for TouchEvent and Touch constructors w3c/touch-events#27

Closed

chong-z mentioned this pull request Nov 4, 2015

Import and replace Touch&TouchEvent constructor tests from w3c w3c/touch-events#45

Closed

tobie added touch-events wg-webevents labels Nov 19, 2015

AFBarstow mentioned this pull request Nov 25, 2015

Remove tests from this repository in favour of wpt. w3c/touch-events#46

Closed

plehegar added reviewed stale:awaiting-submitter-response labels Feb 5, 2016

Ms2ger reviewed Feb 25, 2016
View reviewed changes

chong-z force-pushed the touch-touchevent-constructor branch from ad60668 to 354a82d Compare February 25, 2016 17:35

jdm removed the stale:awaiting-submitter-response label Feb 26, 2016

jdm added a commit that referenced this pull request Feb 26, 2016

Merge pull request #2299 from choniong/touch-touchevent-constructor

1bfaa98

Added constructor tests for Touch and TouchEvent

jdm merged commit 1bfaa98 into web-platform-tests:master Feb 26, 2016

chong-z deleted the touch-touchevent-constructor branch March 8, 2016 23:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added constructor tests for Touch and TouchEvent #2299

Added constructor tests for Touch and TouchEvent #2299

chong-z commented Nov 3, 2015

hoppipolla-critic-bot commented Nov 3, 2015

RByers commented Nov 3, 2015

RByers commented Nov 3, 2015

AFBarstow commented Nov 25, 2015

jgraham commented Dec 23, 2015

RByers commented Dec 23, 2015

RByers commented Dec 23, 2015

RByers commented Dec 23, 2015

Ms2ger commented Dec 23, 2015

RByers commented Dec 23, 2015

RByers commented Dec 23, 2015

Ms2ger commented Dec 23, 2015

chong-z commented Dec 23, 2015

RByers commented Dec 23, 2015

RByers commented Dec 23, 2015

chong-z commented Feb 18, 2016

chong-z commented Feb 22, 2016

Ms2ger Feb 25, 2016

Ms2ger commented Feb 25, 2016

chong-z commented Feb 25, 2016

RByers commented Feb 25, 2016

chong-z commented Feb 25, 2016

jdm commented Feb 26, 2016

Added constructor tests for Touch and TouchEvent #2299

Added constructor tests for Touch and TouchEvent #2299

Conversation

chong-z commented Nov 3, 2015

hoppipolla-critic-bot commented Nov 3, 2015

RByers commented Nov 3, 2015

RByers commented Nov 3, 2015

AFBarstow commented Nov 25, 2015

jgraham commented Dec 23, 2015

RByers commented Dec 23, 2015

RByers commented Dec 23, 2015

RByers commented Dec 23, 2015

Ms2ger commented Dec 23, 2015

RByers commented Dec 23, 2015

RByers commented Dec 23, 2015

Ms2ger commented Dec 23, 2015

chong-z commented Dec 23, 2015

RByers commented Dec 23, 2015

RByers commented Dec 23, 2015

chong-z commented Feb 18, 2016

chong-z commented Feb 22, 2016

Ms2ger Feb 25, 2016

Choose a reason for hiding this comment

Ms2ger commented Feb 25, 2016

chong-z commented Feb 25, 2016

RByers commented Feb 25, 2016

chong-z commented Feb 25, 2016

jdm commented Feb 26, 2016