Bug 94133

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:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

90451

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none

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	Details Formatted Diff Diff
Patch (4.84 KB, patch) 2012-08-15 12:01 PDT, Chris Dumez	no flags	Details Formatted Diff Diff
Patch (4.84 KB, patch) 2012-08-15 21:59 PDT, Chris Dumez	no flags	Details Formatted Diff Diff
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.