LayoutTests/vibration/cancelVibration-during-pattern-vibrating.html - Issue 18478003: Vibration cannot be canceled during pattern vibration.

Side by Side Diff: LayoutTests/vibration/cancelVibration-during-pattern-vibrating.html

Issue 18478003: Vibration cannot be canceled during pattern vibration. (Closed) Base URL: https://chromium.googlesource.com/chromium/blink.git@master

Patch Set: Created 7 years, 5 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View unified diff | Download patch

« no previous file with comments | « LayoutTests/fast/dom/navigator-vibration-expected.txt ('k') | LayoutTests/vibration/cancelVibration-during-pattern-vibrating-expected.txt » ('j') | Source/modules/vibration/NavigatorVibration.cpp » ('J')
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

OLD	NEW
(Empty)
	1 <html>

	2 <head>

	3 <script src="../fast/js/resources/js-test-pre.js"></script>

	4 </head>

	5 <body>

	6 <script>

	7 description('Tests the cancelVibration during pattern vibration is working.');

	8

	9 var count = 0;

	10 var cancelingTime = 80;

	11

	12 if (window.testRunner) {

	13 function vibrate() {

	14 navigator.vibrate([100, 100, 100, 100, 100]);
	Michael van Ouwerkerk 2013/07/03 10:47:55 I find this test really hard to read, with a count I find this test really hard to read, with a counter and calculated timeouts based on that counter. Also it lacks comments and does not test whether isVibrating becomes false when the pattern ends. Please perform separate simpler tests, and explicitly comment on the purpose of the separate tests: // Test that cancelling works during a vibration in a pattern. shouldBeFalse('internals.isVibrating()'); navigator.vibrate([100, 100, 100]); // Progress time by 50ms so we are in the middle of the first vibration of the pattern. shouldBeTrue('internals.isVibrating()'); navigator.vibrate(0); shouldBeFalse('internals.isVibrating()'); // Test that cancelling works during a pause in a pattern. shouldBeFalse('internals.isVibrating()'); navigator.vibrate([100, 100, 100]); // Progress time by 150ms so we are in the middle of the first pause of the pattern. shouldBeTrue('internals.isVibrating()'); navigator.vibrate(0); shouldBeFalse('internals.isVibrating()'); // Test that the system stops thinking that it is vibrating when the pattern ends. shouldBeFalse('internals.isVibrating()'); navigator.vibrate([100, 100, 100]); shouldBeTrue('internals.isVibrating()'); // Progress time by 350ms so the pattern ends naturally without interruptions. shouldBeFalse('internals.isVibrating()'); // Test that a trailing pause is stripped so isVibrating becomes false after the last vibration in the pattern. shouldBeFalse('internals.isVibrating()'); navigator.vibrate([100, 100, 100, 100]); // Even number of array entries, the trailing pause will be stripped. shouldBeTrue('internals.isVibrating()'); // Progress time by 350ms so the pattern ends naturally without interruptions. shouldBeFalse('internals.isVibrating()'); kihong 2013/07/08 10:14:06 Thanks for your comments. I'm changing my tests mo Show quoted text On 2013/07/03 10:47:55, Michael van Ouwerkerk wrote: > I find this test really hard to read, with a counter and calculated timeouts > based on that counter. Also it lacks comments and does not test whether > isVibrating becomes false when the pattern ends. Please perform separate simpler > tests, and explicitly comment on the purpose of the separate tests: > > // Test that cancelling works during a vibration in a pattern. > shouldBeFalse('internals.isVibrating()'); > navigator.vibrate([100, 100, 100]); > // Progress time by 50ms so we are in the middle of the first vibration of the > pattern. > shouldBeTrue('internals.isVibrating()'); > navigator.vibrate(0); > shouldBeFalse('internals.isVibrating()'); > > // Test that cancelling works during a pause in a pattern. > shouldBeFalse('internals.isVibrating()'); > navigator.vibrate([100, 100, 100]); > // Progress time by 150ms so we are in the middle of the first pause of the > pattern. > shouldBeTrue('internals.isVibrating()'); > navigator.vibrate(0); > shouldBeFalse('internals.isVibrating()'); > > // Test that the system stops thinking that it is vibrating when the pattern > ends. > shouldBeFalse('internals.isVibrating()'); > navigator.vibrate([100, 100, 100]); > shouldBeTrue('internals.isVibrating()'); > // Progress time by 350ms so the pattern ends naturally without interruptions. > shouldBeFalse('internals.isVibrating()'); > > // Test that a trailing pause is stripped so isVibrating becomes false after the > last vibration in the pattern. > shouldBeFalse('internals.isVibrating()'); > navigator.vibrate([100, 100, 100, 100]); // Even number of array entries, the > trailing pause will be stripped. > shouldBeTrue('internals.isVibrating()'); > // Progress time by 350ms so the pattern ends naturally without interruptions. > shouldBeFalse('internals.isVibrating()'); > Thanks for your comments. I'm changing my tests more simple.
	15 shouldBeTrue('internals.isVibrating()');

	16 cancelVibration(cancelingTime + count * 100);

	17 }

	18

	19 function cancelVibration(time) {

	20 setTimeout(function() {
	Michael van Ouwerkerk 2013/07/03 10:47:55 Tests should not wait for timeouts, they will run Tests should not wait for timeouts, they will run too long. There should be some code in the animation tests that handles this better. kihong 2013/07/08 10:14:06 I changed my tests but I don't understand what you Show quoted text On 2013/07/03 10:47:55, Michael van Ouwerkerk wrote: > Tests should not wait for timeouts, they will run too long. There should be some > code in the animation tests that handles this better. I changed my tests but I don't understand what you said exactly, is there a way to wait without setTimeout()? Michael van Ouwerkerk 2013/07/10 16:10:05 The tests are much easier to read now, thank you. The tests are much easier to read now, thank you. In previous projects (not Blink / Chromium) we used mocks / fakes for time, or we would override setTimeout. Using these tools we could simply set time (synchronously) during tests, and we didn't have to wait for time to pass or make tests asynchronous. I expect something similar exists in Blink, but I don't know where, sorry. Maybe someone more experienced with Blink knows a good example? On 2013/07/08 10:14:06, kihong wrote: Show quoted text > On 2013/07/03 10:47:55, Michael van Ouwerkerk wrote: > > Tests should not wait for timeouts, they will run too long. There should be > some > > code in the animation tests that handles this better. > > I changed my tests but I don't understand what you said exactly, is there a way > to wait without setTimeout()?
	21 navigator.vibrate(0);

	22 shouldBeFalse('internals.isVibrating()');

	23 if (count++ == 2)

	24 finishJSTest();

	25 vibrate();

	26 }, time);

	27 }

	28

	29 shouldBeFalse('internals.isVibrating()');

	30 vibrate();

	31 window.jsTestIsAsync = true;

	32 } else
	Michael van Ouwerkerk 2013/07/03 10:47:55 Please use curly braces for the else clause, becau Please use curly braces for the else clause, because the if clause also has them. kihong 2013/07/08 10:14:06 Done. Show quoted text On 2013/07/03 10:47:55, Michael van Ouwerkerk wrote: > Please use curly braces for the else clause, because the if clause also has > them. Done.
	33 debug('This test can not be run without the TestRunner');

	34 </script>

	35 <script src="../fast/js/resources/js-test-post.js"></script>

	36 </body>

	37 </html>

OLD	NEW