Bug 91983 – [EFL][WK2] Add unit tests for custom text encoding setting

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Sudarsana Nagineni (babu)]

Add unit tests for custom text encoding setting.

[Sudarsana Nagineni (babu)]

Created attachment 153781 [details]
Patch

[Sudarsana Nagineni (babu)]

Created attachment 153850 [details]
Patch

Add unit tests for set/get custom text encoding.

[Chris Dumez]

Comment on attachment 153850 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:39
> +    ASSERT_STREQ(ewk_view_setting_encoding_custom_get(webView()), 0);

Should we really use ASSERT_STREQ() for 0? Why not use ASSERT_FALSE(ewk_view_setting_encoding_custom_get(webView())); ?

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:40
> +    ewk_view_setting_encoding_custom_set(webView(), "UTF-8");

Would be nice to test the return value.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:43
> +    ewk_view_setting_encoding_custom_set(webView(), 0);

Ditto.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:44
> +    ASSERT_STREQ(ewk_view_setting_encoding_custom_get(webView()), 0);

Should we really use ASSERT_STREQ() for 0? Why not use ASSERT_FALSE(ewk_view_setting_encoding_custom_get(webView())); ?

[Sudarsana Nagineni (babu)]

Comment on attachment 153850 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:39
>> +    ASSERT_STREQ(ewk_view_setting_encoding_custom_get(webView()), 0);
> 
> Should we really use ASSERT_STREQ() for 0? Why not use ASSERT_FALSE(ewk_view_setting_encoding_custom_get(webView())); ?

ASSERT_FALSE also works in this case, but is there anything wrong in using ASSERT_STREQ() here? *_get() returns a C string and I want to assert if C string is NULL(like explained in docs ASSERT_STREQ(NULL, c_string)). But, style checker pushed me to use 0 instead of NULL.

>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:40
>> +    ewk_view_setting_encoding_custom_set(webView(), "UTF-8");
> 
> Would be nice to test the return value.

I wonder how much it makes sense to test the return value of set function, because it returns always true(except in case of invalid input arg). That's the reason I am calling get immediately after set and comparing the C string.

[Chris Dumez]

Comment on attachment 153850 [details]
Patch

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

>>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:39
>>> +    ASSERT_STREQ(ewk_view_setting_encoding_custom_get(webView()), 0);
>> 
>> Should we really use ASSERT_STREQ() for 0? Why not use ASSERT_FALSE(ewk_view_setting_encoding_custom_get(webView())); ?
> 
> ASSERT_FALSE also works in this case, but is there anything wrong in using ASSERT_STREQ() here? *_get() returns a C string and I want to assert if C string is NULL(like explained in docs ASSERT_STREQ(NULL, c_string)). But, style checker pushed me to use 0 instead of NULL.

I don't know if it's wrong but at least it is confusing.

>>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:40
>>> +    ewk_view_setting_encoding_custom_set(webView(), "UTF-8");
>> 
>> Would be nice to test the return value.
> 
> I wonder how much it makes sense to test the return value of set function, because it returns always true(except in case of invalid input arg). That's the reason I am calling get immediately after set and comparing the C string.

Well, better safe than sorry. Just because the current implementation always returns TRUE, does not mean the code will stay this way. It is always a good idea to test as much as possible. In this case, it is very easy to add an ASSERT_TRUE() and make sure the return value is really true (and stays that way in the future).

[Sudarsana Nagineni (babu)]

Comment on attachment 153850 [details]
Patch

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

>>>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:40
>>>> +    ewk_view_setting_encoding_custom_set(webView(), "UTF-8");
>>> 
>>> Would be nice to test the return value.
>> 
>> I wonder how much it makes sense to test the return value of set function, because it returns always true(except in case of invalid input arg). That's the reason I am calling get immediately after set and comparing the C string.
> 
> Well, better safe than sorry. Just because the current implementation always returns TRUE, does not mean the code will stay this way. It is always a good idea to test as much as possible. In this case, it is very easy to add an ASSERT_TRUE() and make sure the return value is really true (and stays that way in the future).

Yes, it is easy to add and I don't mind adding, but I'm just trying to understand the real use of it with the current implementation. Isn't it true that we should update the test code when we change the APIs implementation?

[Sudarsana Nagineni (babu)]

Created attachment 153874 [details]
Patch

Fix review comment #3.

[Gyuyoung Kim]

Comment on attachment 153874 [details]
Patch

LGTM.

[WebKit Review Bot]

Comment on attachment 153874 [details]
Patch

Clearing flags on attachment: 153874

Committed r123545: <http://trac.webkit.org/changeset/123545>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.