Bug 118123

Summary: [EFL][WK2] Add spellcheck API tests in unit tests.
Product: WebKit Reporter: Dariusz Frankiewicz <d.frankiewic>
Component: Tools / TestsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Enhancement CC: cdumez, commit-queue, g.czajkowski, gyuyoung.kim, rakuco
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Proposed patch
cdumez: review-
Proposed patch version 2
none
Proposed patch version 3
cdumez: review+, cdumez: commit-queue-
Proposed patch version 4
none
Archive of layout-test-results from APPLE-EWS-1 for win-future none

Description Dariusz Frankiewicz 2013-06-27 03:26:25 PDT 
As a result of discussion in bug https://bugs.webkit.org/show_bug.cgi?id=113742 we're moving spelling tests from layout tests to unit tests.
Comment 1 Dariusz Frankiewicz 2013-06-27 03:34:52 PDT 
Created attachment 205583 [details]
Proposed patch
Comment 2 Chris Dumez 2013-06-27 03:45:47 PDT 
Comment on attachment 205583 [details]
Proposed patch

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

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:54
> +static size_t contextMenuItemsNumber = 0;

Please use unsigned as this is the type returned by eina_list_count().

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:329
> +    size_t numberItemsWithoutSpellCheck = contextMenuItemsNumber;

unsigned

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:338
> +    size_t numberItemsWithSpellCheck = contextMenuItemsNumber;

unsigned

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:342
> +    wasContextMenuShown = false;

I don't think we should reset the boolean at the end of the test. The next test should set the boolean value before using it instead.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:371
> +    wasContextMenuShown = false;

Ditto.
Comment 3 Chris Dumez 2013-06-27 03:50:24 PDT 
Comment on attachment 205583 [details]
Proposed patch

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

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:328
> +    waitUntilTrue(wasContextMenuShown);

You need to ASSERT_TRUE iirc.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:337
> +    waitUntilTrue(wasContextMenuShown);

You need to ASSERT_TRUE iirc.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:356
> +    waitUntilTrue(wasContextMenuShown);

You need to ASSERT_TRUE iirc.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:357
> +    size_t numberItemsWithoutSpellCheck = contextMenuItemsNumber;

unsigned.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:361
> +    // Testing how many items are in context menu when multiword are selected.

"multiple words"

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:366
> +    waitUntilTrue(wasContextMenuShown);

You need to ASSERT_TRUE iirc.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:367
> +    size_t numberItemsWithSpellCheck = contextMenuItemsNumber;

unsigned

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:385
> +    waitUntilTrue(wasContextMenuShown);

You need to ASSERT_TRUE iirc.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:386
> +    size_t numberItemsWithoutSpellCheck = contextMenuItemsNumber;

unsigned

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:395
> +    waitUntilTrue(wasContextMenuShown);

You need to ASSERT_TRUE iirc.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:396
> +    size_t numberItemsWithSpellCheck = contextMenuItemsNumber;

unsigned

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:400
> +    wasContextMenuShown = false;

boolean reset

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:414
> +    waitUntilTrue(wasContextMenuShown);

You need to ASSERT_TRUE iirc.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:415
> +    size_t numberItemsWithoutSpellCheck = contextMenuItemsNumber;

unsigned
Comment 4 Dariusz Frankiewicz 2013-06-27 05:29:46 PDT 
Created attachment 205592 [details]
Proposed patch version 2
Comment 5 WebKit Commit Bot 2013-06-27 05:31:13 PDT 
Attachment 205592 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/UIProcess/API/efl/tests/resources/spelling_selection_tests.html', u'Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp']" exit_code: 1
Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:54:  Omit int when using unsigned  [runtime/unsigned] [1]
Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:331:  Omit int when using unsigned  [runtime/unsigned] [1]
Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:340:  Omit int when using unsigned  [runtime/unsigned] [1]
Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:350:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:359:  Omit int when using unsigned  [runtime/unsigned] [1]
Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:369:  Omit int when using unsigned  [runtime/unsigned] [1]
Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:379:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:388:  Omit int when using unsigned  [runtime/unsigned] [1]
Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:398:  Omit int when using unsigned  [runtime/unsigned] [1]
Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:408:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:417:  Omit int when using unsigned  [runtime/unsigned] [1]
Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:428:  Omit int when using unsigned  [runtime/unsigned] [1]
Total errors found: 12 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Dariusz Frankiewicz 2013-06-27 05:39:42 PDT 
Created attachment 205594 [details]
Proposed patch version 3
Comment 7 Chris Dumez 2013-06-27 05:44:06 PDT 
Comment on attachment 205594 [details]
Proposed patch version 3

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

Looks fine. r=me with nits. Please fix the nits and let Grzegorz take a look before landing.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:340
> +    unsigned numberItemsWithSpellCheck = contextMenuItemsNumber;

nit: Local variable not needed.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:369
> +    unsigned numberItemsWithSpellCheck = contextMenuItemsNumber;

nit: Local variable not needed.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:398
> +    unsigned numberItemsWithSpellCheck = contextMenuItemsNumber;

nit: Local variable not needed.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:428
> +    unsigned numberItemsWithSpellCheck = contextMenuItemsNumber;

nit: Local variable not needed.
Comment 8 Chris Dumez 2013-06-27 05:53:06 PDT 
Comment on attachment 205594 [details]
Proposed patch version 3

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

> Source/WebKit2/ChangeLog:3
> +        Porting spellcheck tests from layout tests to unit tests.

Also, would you change the title? It sounds like you're removing layout tests and adding API tests instead. Just explain what kind of API test you're adding and leave layout tests out of this.
Comment 9 Dariusz Frankiewicz 2013-06-27 07:33:49 PDT 
Created attachment 205603 [details]
Proposed patch version 4
Comment 10 Build Bot 2013-06-27 12:04:44 PDT 
Comment on attachment 205603 [details]
Proposed patch version 4

Attachment 205603 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/854431

New failing tests:
fast/js/global-constructors-attributes-shared-worker.html
fast/js/global-constructors-attributes-dedicated-worker.html
Comment 11 Build Bot 2013-06-27 12:04:46 PDT 
Created attachment 205623 [details]
Archive of layout-test-results from APPLE-EWS-1 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: APPLE-EWS-1  Port: win-future  Platform: CYGWIN_NT-6.1-WOW64-1.7.20-0.266-5-3-i686-32bit
Comment 12 Grzegorz Czajkowski 2013-06-28 00:14:02 PDT 
Comment on attachment 205603 [details]
Proposed patch version 4

Nothing comes to mind after Chris' review :) Thanks guys for supporting those tests!
Comment 13 WebKit Commit Bot 2013-06-28 00:35:16 PDT 
Comment on attachment 205603 [details]
Proposed patch version 4

Clearing flags on attachment: 205603

Committed r152153: <http://trac.webkit.org/changeset/152153>