Bug 119085 – [EFL][WK2] Simplify context_menu_spelling_items_availability unit test

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Grzegorz Czajkowski]

Use findContextMenuItem method to check whether the item appears in context menu.

[Grzegorz Czajkowski]

Created attachment 207454 [details]
proposed patch

[Gyuyoung Kim]

Comment on attachment 207454 [details]
proposed patch

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

LGTM.

> Source/WebKit2/ChangeLog:13
> +        Do not report failure here. It doesn't allow to test the negative test cases.

Shouldn't you need to check if existing uses of findContextMenuItem() are also failed ?

[Gyuyoung Kim]

(In reply to comment #2)
> (From update of attachment 207454 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=207454&action=review
> 
> LGTM.

LGTM except for my question.

[Grzegorz Czajkowski]

Comment on attachment 207454 [details]
proposed patch

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

>> Source/WebKit2/ChangeLog:13
>> +        Do not report failure here. It doesn't allow to test the negative test cases.
> 
> Shouldn't you need to check if existing uses of findContextMenuItem() are also failed ?

Present findContextMenuItem is being used to test the positive test cases only. Therefore it doesn't reach the 'error block' (ADD_FAILURE; return 0;)
IMHO we should check result of findContextMenuItem in the test/callback instead of reporting failure inside. It prevents us to use the method to test negative test cases.

Actually, the unit tests use findContextMenuItem to find an appropriate context menu item and to select it. The result of selected item is verified inside the tests. Assuming the positive test case, if findContextMenuItem fails the test will fail as well - the item won't be selected and the expected result will be different as well.

The reason of introducing ADD_FAILURE() in findContextMenuItem was to report failure earlier.

[Gyuyoung Kim]

(In reply to comment #4)

> Actually, the unit tests use findContextMenuItem to find an appropriate context menu item and to select it. The result of selected item is verified inside the tests. Assuming the positive test case, if findContextMenuItem fails the test will fail as well - the item won't be selected and the expected result will be different as well.
> 
> The reason of introducing ADD_FAILURE() in findContextMenuItem was to report failure earlier.

I see. As we talk on irc, my question was that we need to check the result of findContextMenuItem() in other functions as well.

[Grzegorz Czajkowski]

Created attachment 207519 [details]
verify return value of each findContextMenuItem call

[Gyuyoung Kim]

Comment on attachment 207519 [details]
verify return value of each findContextMenuItem call

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

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:295
> +        EXPECT_TRUE(spellingAndGrammarSubmenu);

Isn't checkSpellingWhileTypingItem instead of spellingAndGrammarSubmenu ?

[Grzegorz Czajkowski]

Comment on attachment 207519 [details]
verify return value of each findContextMenuItem call

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

>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:295
>> +        EXPECT_TRUE(spellingAndGrammarSubmenu);
> 
> Isn't checkSpellingWhileTypingItem instead of spellingAndGrammarSubmenu ?

Ooops. You're right. I'll fix it.

[Grzegorz Czajkowski]

Created attachment 207520 [details]
apply Gyuyoung's review

[Gyuyoung Kim]

Comment on attachment 207520 [details]
apply Gyuyoung's review

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

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:-559
> -        ecore_main_loop_iterate();

When I ask EFL community this function, I could get this reply....
"DO NOT use this function unless you are the person God comes to ask for advice when He has trouble managing the Universe"

Hey, greg, ryuan and chris, let's think about this function. As we know, we are not GOD. ;)

[Gyuyoung Kim]

(In reply to comment #10)
> (From update of attachment 207520 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=207520&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:-559
> > -        ecore_main_loop_iterate();
> 
> When I ask EFL community this function, I could get this reply....
> "DO NOT use this function unless you are the person God comes to ask for advice when He has trouble managing the Universe"

"DO NOT use this function unless you are the person God comes to ask for advice when He has trouble managing the Universe"

Frankly speaking, I got this comment from EFL docs, not efl irc or mailing list...........Plz let me know if this comment is incorrect.

[Chris Dumez]

(In reply to comment #11)
> (In reply to comment #10)
> > (From update of attachment 207520 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=207520&action=review
> > 
> > > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:-559
> > > -        ecore_main_loop_iterate();
> > 
> > When I ask EFL community this function, I could get this reply....
> > "DO NOT use this function unless you are the person God comes to ask for advice when He has trouble managing the Universe"
> 
> "DO NOT use this function unless you are the person God comes to ask for advice when He has trouble managing the Universe"
> 
> Frankly speaking, I got this comment from EFL docs, not efl irc or mailing list...........Plz let me know if this comment is incorrect.

We are using this function in the API tests, I don't think this is a big deal.
Anyway, this part of the patch does not really get rid of ecore_main_loop_iterate(), it merely hides it.

[Grzegorz Czajkowski]

Comment on attachment 207520 [details]
apply Gyuyoung's review

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

>>>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:-559
>>>> -        ecore_main_loop_iterate();
>>> 
>>> When I ask EFL community this function, I could get this reply....
>>> "DO NOT use this function unless you are the person God comes to ask for advice when He has trouble managing the Universe"
>>> 
>>> Hey, greg, ryuan and chris, let's think about this function. As we know, we are not GOD. ;)
>> 
>> "DO NOT use this function unless you are the person God comes to ask for advice when He has trouble managing the Universe"
>> 
>> Frankly speaking, I got this comment from EFL docs, not efl irc or mailing list...........Plz let me know if this comment is incorrect.
> 
> We are using this function in the API tests, I don't think this is a big deal.
> Anyway, this part of the patch does not really get rid of ecore_main_loop_iterate(), it merely hides it.

Yeah, using ecore_main_loop_iterate() in unit tests shouldn't affect the Universe ;)
As Chris said, we are using it in many aspects and replacing it with waitUntilTrue() is rather to match consistency with other tests which already use it.

[Gyuyoung Kim]

(In reply to comment #13)

> > We are using this function in the API tests, I don't think this is a big deal.
> > Anyway, this part of the patch does not really get rid of ecore_main_loop_iterate(), it merely hides it.
> 
> Yeah, using ecore_main_loop_iterate() in unit tests shouldn't affect the Universe ;)
> As Chris said, we are using it in many aspects and replacing it with waitUntilTrue() is rather to match consistency with other tests which already use it.

Ok, let's replace it with waitUntilTrue() from now on.

[Grzegorz Czajkowski]

Committed r153419: <http://trac.webkit.org/changeset/153419>