Bug 118123 - [EFL][WK2] Add spellcheck API tests in unit tests.
Summary: [EFL][WK2] Add spellcheck API tests in unit tests.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Enhancement
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-06-27 03:26 PDT by Dariusz Frankiewicz
Modified: 2013-06-28 03:15 PDT (History)
5 users (show)

See Also:


Attachments
Proposed patch (9.45 KB, patch)
2013-06-27 03:34 PDT, Dariusz Frankiewicz
cdumez: review-
Details | Formatted Diff | Diff
Proposed patch version 2 (9.60 KB, patch)
2013-06-27 05:29 PDT, Dariusz Frankiewicz
no flags Details | Formatted Diff | Diff
Proposed patch version 3 (9.58 KB, patch)
2013-06-27 05:39 PDT, Dariusz Frankiewicz
cdumez: review+
cdumez: commit-queue-
Details | Formatted Diff | Diff
Proposed patch version 4 (9.30 KB, patch)
2013-06-27 07:33 PDT, Dariusz Frankiewicz
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from APPLE-EWS-1 for win-future (813.69 KB, application/zip)
2013-06-27 12:04 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
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>