Bug 99168 - Add tests for suggestion picker that test step/min/max attributes
Summary: Add tests for suggestion picker that test step/min/max attributes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Keishi Hattori
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-10-12 05:46 PDT by Keishi Hattori
Modified: 2012-10-15 20:43 PDT (History)
2 users (show)

See Also:


Attachments
Patch (20.53 KB, patch)
2012-10-12 05:58 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
Patch (930.92 KB, patch)
2012-10-14 19:30 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
Patch (19.96 KB, patch)
2012-10-14 20:55 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
Patch (19.78 KB, patch)
2012-10-15 03:17 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
Patch (19.78 KB, patch)
2012-10-15 19:08 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Keishi Hattori 2012-10-12 05:46:02 PDT
Add tests for suggestion picker that test step/min/max attributes
Comment 1 Keishi Hattori 2012-10-12 05:58:54 PDT
Created attachment 168406 [details]
Patch
Comment 2 Build Bot 2012-10-12 06:21:49 PDT
Comment on attachment 168406 [details]
Patch

Attachment 168406 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14255935

New failing tests:
inspector/profiler/heap-snapshot.html
Comment 3 Kent Tamura 2012-10-14 18:42:02 PDT
Comment on attachment 168406 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=168406&action=review

> LayoutTests/fast/forms/resources/suggestion-picker-common.js:28
> +    for (var i = 0; i < elements.length; ++i) {
> +        values.push(valueForEntry(elements[i]));
> +    }

nit: { and } are not needed

> LayoutTests/platform/chromium-android/TestExpectations:92
> +platform/chromium/fast/forms/date/date-suggestion-picker-min-max-attribute.html [ WontFix ]
> +platform/chromium/fast/forms/date/date-suggestion-picker-step-attribute.html [ WontFix ]
> +platform/chromium/fast/forms/time/time-suggestion-picker-min-max-attribute.html [ WontFix ]
> +platform/chromium/fast/forms/time/time-suggestion-picker-step-attribute.html [ WontFix ]

We had better put *-suggestion-picker-* tests into their own directory.  platform/chromium/fast/forms/suggestion-picker/ ?

> LayoutTests/platform/chromium/fast/forms/date/date-suggestion-picker-min-max-attribute.html:26
> +description("Tests that key bindings work as expected.");
> +
> +debug('Check that page popup doesn\'t exist at first.');

Usage of ' and " is inconsistent.

> LayoutTests/platform/chromium/fast/forms/date/date-suggestion-picker-min-max-attribute.html:31
> +    openPicker(document.getElementById('date'));
> +    popupWindow.addEventListener("resize", test, false);

nit:
We had better introduce openPicker() with a callback argument. like:

function openPicker(element, callback) {
 ....
  popupOpwnCallback = callback;
  popupWindow.addEventListener("resize", pickerCallbackWrapper, false);
}

function pickerCallbackWrapper() {
    popupWindow.removeEventListener("resize", pickerCallbackWrapper);
    popupOpenCallback();
}

> LayoutTests/platform/chromium/fast/forms/date/date-suggestion-picker-min-max-attribute.html:37
> +var timeoutTimer = setTimeout(function() {
> +    testFailed("Test timed out.");
> +    finishJSTest();
> +}, 10000);

Do not add time out detection in tests.  It will be a source of flakiness.

> LayoutTests/platform/chromium/fast/forms/date/date-suggestion-picker-step-attribute.html:39
> +var timeoutTimer = setTimeout(function() {
> +    testFailed("Test timed out.");
> +    finishJSTest();
> +}, 10000);

ditto.

> LayoutTests/platform/chromium/fast/forms/time/time-suggestion-picker-min-max-attribute.html:34
> +var timeoutTimer = setTimeout(function() {
> +    testFailed("Test timed out.");
> +    finishJSTest();
> +}, 10000);

ditto.

> LayoutTests/platform/chromium/fast/forms/time/time-suggestion-picker-step-attribute.html:36
> +var timeoutTimer = setTimeout(function() {
> +    testFailed("Test timed out.");
> +    finishJSTest();
> +}, 10000);

ditto.
Comment 4 Keishi Hattori 2012-10-14 19:30:50 PDT
Created attachment 168610 [details]
Patch
Comment 5 Kent Tamura 2012-10-14 20:11:05 PDT
Comment on attachment 168610 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=168610&action=review

> LayoutTests/ChangeLog:9
> +        I am also moving the suggestion picker tests to platform/chromium/fast/forms/suggestion-picker directory.

Please split the patch.
Comment 6 Keishi Hattori 2012-10-14 20:55:26 PDT
Created attachment 168618 [details]
Patch
Comment 7 WebKit Review Bot 2012-10-14 21:01:27 PDT
Attachment 168618 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
LayoutTests/platform/chromium-android/TestExpectations:72:  Test lacks BUG modifier.  [test/expectations] [5]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 Kent Tamura 2012-10-14 21:01:51 PDT
Comment on attachment 168618 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=168618&action=review

> LayoutTests/platform/chromium/fast/forms/suggestion-picker/date-suggestion-picker-min-max-attribute.html:35
> +    popupWindow.removeEventListener("resize", test, false);

removeEventListener is unnecessary.

> LayoutTests/platform/chromium/fast/forms/suggestion-picker/date-suggestion-picker-step-attribute.html:33
> +    popupWindow.addEventListener("resize", test1, false);

addEventListener is unnecessary.

> LayoutTests/platform/chromium/fast/forms/suggestion-picker/time-suggestion-picker-min-max-attribute.html:32
> +    popupWindow.removeEventListener("resize", test, false);

removeEventListener is unnecessary.
Comment 9 Keishi Hattori 2012-10-15 03:17:48 PDT
Created attachment 168665 [details]
Patch
Comment 10 Kent Tamura 2012-10-15 03:22:06 PDT
Comment on attachment 168665 [details]
Patch

ok
Comment 11 WebKit Review Bot 2012-10-15 03:25:16 PDT
Attachment 168665 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
LayoutTests/platform/chromium-android/TestExpectations:72:  Test lacks BUG modifier.  [test/expectations] [5]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 12 Keishi Hattori 2012-10-15 19:08:04 PDT
Created attachment 168837 [details]
Patch
Comment 13 WebKit Review Bot 2012-10-15 19:29:24 PDT
Comment on attachment 168837 [details]
Patch

Rejecting attachment 168837 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
51>At revision 154708.

________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...
LayoutTests/platform/chromium-android/TestExpectations:72:  Test lacks BUG modifier.  [test/expectations] [5]
Total errors found: 1 in 1 files

Full output: http://queues.webkit.org/results/14297834
Comment 14 Keishi Hattori 2012-10-15 20:43:39 PDT
Comment on attachment 168837 [details]
Patch

Clearing flags on attachment: 168837

Committed r131404: <http://trac.webkit.org/changeset/131404>
Comment 15 Keishi Hattori 2012-10-15 20:43:43 PDT
All reviewed patches have been landed.  Closing bug.