Summary: | [EFL][WK2] Add unit tests for Web intent registration | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||||
Component: | WebKit2 | Assignee: | Chris Dumez <cdumez> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | gyuyoung.kim, kenneth, rakuco, tmpsantos, tonikitoo, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 90451 | ||||||||||
Attachments: |
|
Description
Chris Dumez
2012-08-15 11:47:25 PDT
Created attachment 158609 [details]
Patch
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()? Created attachment 158612 [details]
Patch
Remove "onload", Thanks Thiago.
Comment on attachment 158612 [details]
Patch
LGTM. Thanks!
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? (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 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. 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? 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. (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. (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. Created attachment 158707 [details]
Patch
Comment on attachment 158707 [details] Patch Clearing flags on attachment: 158707 Committed r125762: <http://trac.webkit.org/changeset/125762> All reviewed patches have been landed. Closing bug. |