| Summary: | [EFL][WK2] Simplify context_menu_spelling_items_availability unit test | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Grzegorz Czajkowski <g.czajkowski> | ||||||||
| Component: | WebKit EFL | Assignee: | Grzegorz Czajkowski <g.czajkowski> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | cdumez, commit-queue, gyuyoung.kim, gyuyoung.kim, lucas.de.marchi, rakuco | ||||||||
| Priority: | P2 | ||||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Grzegorz Czajkowski
2013-07-25 06:36:29 PDT
Created attachment 207454 [details]
proposed patch
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 ? (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. 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. (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. Created attachment 207519 [details]
verify return value of each findContextMenuItem call
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 ? 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. Created attachment 207520 [details]
apply Gyuyoung's review
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. ;) (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. (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. 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. (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. Committed r153419: <http://trac.webkit.org/changeset/153419> |