Bug 118123 – [EFL][WK2] Add spellcheck API tests in unit tests.

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Dariusz Frankiewicz]

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.

[Dariusz Frankiewicz]

Created attachment 205583 [details]
Proposed patch

[Chris Dumez]

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.

[Chris Dumez]

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

[Dariusz Frankiewicz]

Created attachment 205592 [details]
Proposed patch version 2

[WebKit Commit Bot]

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.

[Dariusz Frankiewicz]

Created attachment 205594 [details]
Proposed patch version 3

[Chris Dumez]

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.

[Chris Dumez]

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.

[Dariusz Frankiewicz]

Created attachment 205603 [details]
Proposed patch version 4

[Build Bot]

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

[Build Bot]

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

[Grzegorz Czajkowski]

Comment on attachment 205603 [details]
Proposed patch version 4

Nothing comes to mind after Chris' review :) Thanks guys for supporting those tests!

[WebKit Commit Bot]

Comment on attachment 205603 [details]
Proposed patch version 4

Clearing flags on attachment: 205603

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