RESOLVED FIXED 94133
[EFL][WK2] Add unit tests for Web intent registration 
https://bugs.webkit.org/show_bug.cgi?id=94133
Summary [EFL][WK2] Add unit tests for Web intent registration
Chris Dumez
Reported 2012-08-15 11:47:25 PDT
We need unit tests for Web intent registration. This would test the "intent,service,register" signalling on the Ewk_View as well as Ewk_Intent_Service.
Attachments
Patch (4.86 KB, patch)
2012-08-15 11:50 PDT, Chris Dumez 		no flags
DetailsFormatted DiffDiff
Patch (4.84 KB, patch)
2012-08-15 12:01 PDT, Chris Dumez 		no flags
DetailsFormatted DiffDiff
Patch (4.84 KB, patch)
2012-08-15 21:59 PDT, Chris Dumez 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Chris Dumez
Comment 1 2012-08-15 11:50:50 PDT
Created attachment 158609 [details] Patch
Thiago Marcos P. Santos
Comment 2 2012-08-15 11:57:37 PDT
Comment on attachment 158609 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158609&action=review > Source/WebKit2/UIProcess/API/efl/tests/resources/intent-service.html:5 > + <body onload="loaded()"> where is loaded()?
Chris Dumez
Comment 3 2012-08-15 12:01:36 PDT
Created attachment 158612 [details] Patch Remove "onload", Thanks Thiago.
Thiago Marcos P. Santos
Comment 4 2012-08-15 12:13:09 PDT
Comment on attachment 158612 [details] Patch LGTM. Thanks!
Kenneth Rohde Christiansen
Comment 5 2012-08-15 12:25:29 PDT
Comment on attachment 158612 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158612&action=review > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_intents.cpp:45 > + ASSERT_TRUE(service); > + ASSERT_STREQ(ewk_intent_service_action_get(service), "action"); Where do I find information about these macros an this test system?
Chris Dumez
Comment 6 2012-08-15 12:29:38 PDT
(In reply to comment #5) > (From update of attachment 158612 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=158612&action=review > > > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_intents.cpp:45 > > + ASSERT_TRUE(service); > > + ASSERT_STREQ(ewk_intent_service_action_get(service), "action"); > > Where do I find information about these macros an this test system? http://trac.webkit.org/wiki/EFLWebKitTests http://code.google.com/p/googletest/wiki/Primer
Thiago Marcos P. Santos
Comment 7 2012-08-15 12:32:52 PDT
Comment on attachment 158612 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158612&action=review >> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_intents.cpp:45 >> + ASSERT_STREQ(ewk_intent_service_action_get(service), "action"); > > Where do I find information about these macros an this test system? These are the docs we have: http://trac.webkit.org/wiki/EFLWebKitTests On the wiki you will find links to gtest docs. gtest is the foundation of our utest framework and where these macros are coming from.
Kenneth Rohde Christiansen
Comment 8 2012-08-15 12:37:05 PDT
Comment on attachment 158612 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158612&action=review > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_intents.cpp:40 > + ASSERT_FALSE(*intentRegistered); So we are supposed to ASSERT and use EXPECTS_ macros when it is actually possible to continue? > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_intents.cpp:47 > + ASSERT_STREQ(ewk_intent_service_type_get(service), "type"); > + ASSERT_STREQ(ewk_intent_service_title_get(service), "Title"); Wouldn't EXPECT_STREQ be better here? to continue testing even if that one fails?
Thiago Marcos P. Santos
Comment 9 2012-08-15 13:20:58 PDT
Comment on attachment 158612 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158612&action=review >> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_intents.cpp:47 >> + ASSERT_STREQ(ewk_intent_service_title_get(service), "Title"); > > Wouldn't EXPECT_STREQ be better here? to continue testing even if that one fails? This is not the first time this topic is brought up. I voted for using assertions only if a failure after that point breaks the test beyond repair. A counter argument was that too many failures messages can be misleading in case of a regression.
Kenneth Rohde Christiansen
Comment 10 2012-08-15 13:59:26 PDT
(In reply to comment #9) > (From update of attachment 158612 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=158612&action=review > > >> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_intents.cpp:47 > >> + ASSERT_STREQ(ewk_intent_service_title_get(service), "Title"); > > > > Wouldn't EXPECT_STREQ be better here? to continue testing even if that one fails? > > This is not the first time this topic is brought up. I voted for using assertions only if a failure after that point breaks the test beyond repair. A counter argument was that too many failures messages can be misleading in case of a regression. Sure, they might be, but they might also not be. At least I think it is good to know if it is the only thing failing or everything else also becomes unreliable. EXPECT_* exists for a purpose.
Chris Dumez
Comment 11 2012-08-15 21:58:38 PDT
(In reply to comment #10) > (In reply to comment #9) > > (From update of attachment 158612 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=158612&action=review > > > > >> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_intents.cpp:47 > > >> + ASSERT_STREQ(ewk_intent_service_title_get(service), "Title"); > > > > > > Wouldn't EXPECT_STREQ be better here? to continue testing even if that one fails? > > > > This is not the first time this topic is brought up. I voted for using assertions only if a failure after that point breaks the test beyond repair. A counter argument was that too many failures messages can be misleading in case of a regression. > > Sure, they might be, but they might also not be. At least I think it is good to know if it is the only thing failing or everything else also becomes unreliable. EXPECT_* exists for a purpose. Agreed, I'll update the patch. Thanks.
Chris Dumez
Comment 12 2012-08-15 21:59:42 PDT
Created attachment 158707 [details] Patch
WebKit Review Bot
Comment 13 2012-08-16 02:13:13 PDT
Comment on attachment 158707 [details] Patch Clearing flags on attachment: 158707 Committed r125762: <http://trac.webkit.org/changeset/125762>
WebKit Review Bot
Comment 14 2012-08-16 02:13:19 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.