Bug 92617 - [EFL][wk2] Add unit tests for back-forward list API
Summary: [EFL][wk2] Add unit tests for back-forward list API
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mikhail Pozdnyakov
URL:
Keywords:
Depends on: 92345 94248
Blocks: 61838 90451
  Show dependency treegraph
 
Reported: 2012-07-30 01:02 PDT by Mikhail Pozdnyakov
Modified: 2012-08-19 02:23 PDT (History)
8 users (show)

See Also:


Attachments
patch (9.77 KB, patch)
2012-08-16 14:07 PDT, Mikhail Pozdnyakov
no flags Details | Formatted Diff | Diff
patch v2 (9.76 KB, patch)
2012-08-17 00:16 PDT, Mikhail Pozdnyakov
kenneth: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-07 (329.85 KB, application/zip)
2012-08-17 03:14 PDT, WebKit Review Bot
no flags Details
to be landed (9.77 KB, patch)
2012-08-17 07:11 PDT, Mikhail Pozdnyakov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mikhail Pozdnyakov 2012-07-30 01:02:23 PDT
Unit tests for back-forward list API should be added.
Comment 1 Thiago Marcos P. Santos 2012-08-07 05:08:50 PDT
Ideally the tests should land together with the feature (or at least be available for review). Are we going to see patches for this one coming soon?
Comment 2 Chris Dumez 2012-08-07 06:28:15 PDT
(In reply to comment #1)
> Ideally the tests should land together with the feature (or at least be available for review). Are we going to see patches for this one coming soon?

Well, this would result in big patches. At least, with a bug report filed, we know the unit tests are coming.
Comment 3 Thiago Marcos P. Santos 2012-08-07 06:48:43 PDT
(In reply to comment #2)
> (In reply to comment #1)
> > Ideally the tests should land together with the feature (or at least be available for review). Are we going to see patches for this one coming soon?
> 
> Well, this would result in big patches. At least, with a bug report filed, we know the unit tests are coming.

I'm fine with separated bugs, but they both should be posted for review at the same time.

Otherwise it become a technical debt that who knows when it is going to be paid.
Comment 4 Mikhail Pozdnyakov 2012-08-16 14:07:59 PDT
Created attachment 158895 [details]
patch

Returning of technical debt :-)
Comment 5 Chris Dumez 2012-08-16 14:18:11 PDT
Comment on attachment 158895 [details]
patch

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

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_back_forward_list.cpp:92
> +    ASSERT_TRUE(currentItem == anotherCurrentItem);

ASSERT_EQ()?

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_back_forward_list.cpp:114
> +    ASSERT_TRUE(previousItem == anotherPreviousItem);

ASSERT_EQ(previousItem, anotherPreviousItem)?

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_back_forward_list.cpp:140
> +    ASSERT_TRUE(nextItem == anotherNextItem);

Ditto.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_back_forward_list.cpp:162
> +    ASSERT_TRUE(previousItem == anotherPreviousItem);

Ditto.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_back_forward_list.cpp:165
> +    ASSERT_TRUE(!nonExistingItem);

ASSERT_FALSE(nonExistingItem)?
Comment 6 Kenneth Rohde Christiansen 2012-08-16 14:27:00 PDT
Comment on attachment 158895 [details]
patch

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

I will look closer at this later, unfortunately it is quite late

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_back_forward_list.cpp:81
> +TEST_F(EWK2UnitTestBase, ewk_back_forward_list_current_item_get)

Why the 2, isnt it pretty obvious given the path that it is a webkit 2 test? thing or normally only prefixed ewk and WK. I also refer to the file name, it looks out of place

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_back_forward_list.cpp:129
> +    // Go back to Page1

Punctuation mark at end
Comment 7 Chris Dumez 2012-08-16 14:33:14 PDT
Comment on attachment 158895 [details]
patch

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

>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_back_forward_list.cpp:81
>> +TEST_F(EWK2UnitTestBase, ewk_back_forward_list_current_item_get)
> 
> Why the 2, isnt it pretty obvious given the path that it is a webkit 2 test? thing or normally only prefixed ewk and WK. I also refer to the file name, it looks out of place

Regarding the file name, this is part of the guidelines for WebKit2-EFL unit tests (http://trac.webkit.org/wiki/EFLWebKitTests):
"Add your test file at: Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_foobarnicator.cpp (the prefix is test_ewk2 and not test_ewk, this was done to avoid clashes with WK1 tests) "
Comment 8 Mikhail Pozdnyakov 2012-08-17 00:16:31 PDT
Created attachment 159021 [details]
patch v2

Chris, Kenneth thanks for review
Comment 9 Chris Dumez 2012-08-17 00:19:53 PDT
Comment on attachment 159021 [details]
patch v2

LGTM. Thanks.
Comment 10 WebKit Review Bot 2012-08-17 03:13:56 PDT
Comment on attachment 159021 [details]
patch v2

Attachment 159021 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13510975

New failing tests:
http/tests/cache/post-redirect-get.php
http/tests/cache/post-with-cached-subresources.php
Comment 11 WebKit Review Bot 2012-08-17 03:14:02 PDT
Created attachment 159070 [details]
Archive of layout-test-results from gce-cr-linux-07

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-07  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment 12 Thiago Marcos P. Santos 2012-08-17 03:27:45 PDT
Comment on attachment 158895 [details]
patch

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

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_back_forward_list.cpp:100
> +    WKEinaSharedString url1 = urlFromTitle(httpServer.get(), title1);

why name url1 if the test has no url2?

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_back_forward_list.cpp:125
> +    WKEinaSharedString url2 = urlFromTitle(httpServer.get(), title2);

where is url1?
Comment 13 Mikhail Pozdnyakov 2012-08-17 03:36:48 PDT
(In reply to comment #12)
> (From update of attachment 158895 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=158895&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_back_forward_list.cpp:100
> > +    WKEinaSharedString url1 = urlFromTitle(httpServer.get(), title1);
> 
> why name url1 if the test has no url2?
> 
> > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_back_forward_list.cpp:125
> > +    WKEinaSharedString url2 = urlFromTitle(httpServer.get(), title2);
> 
> where is url1?

I just meant that url2 corresponds to title2 and same for url1 and title1.
Comment 14 Mikhail Pozdnyakov 2012-08-17 03:38:02 PDT
(In reply to comment #10)
> (From update of attachment 159021 [details])
> Attachment 159021 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/13510975
> 
> New failing tests:
> http/tests/cache/post-redirect-get.php
> http/tests/cache/post-with-cached-subresources.php

Seems to be false positive, patch is adding unit test for EFL WK2 API unrelated to chromium.
Comment 15 Mikhail Pozdnyakov 2012-08-17 07:11:40 PDT
Created attachment 159120 [details]
to be landed

Retry.
Comment 16 WebKit Review Bot 2012-08-17 08:37:36 PDT
Comment on attachment 159120 [details]
to be landed

Clearing flags on attachment: 159120

Committed r125905: <http://trac.webkit.org/changeset/125905>
Comment 17 WebKit Review Bot 2012-08-17 08:37:43 PDT
All reviewed patches have been landed.  Closing bug.
Comment 18 Thiago Marcos P. Santos 2012-08-17 14:36:23 PDT
Something wrong with test (or the API). It is not passing since it landed.
Comment 19 Chris Dumez 2012-08-17 14:39:10 PDT
(In reply to comment #18)
> Something wrong with test (or the API). It is not passing since it landed.

I'm guessing because the dependency (Bug 94248) has not landed?
Comment 20 Thiago Marcos P. Santos 2012-08-19 02:23:06 PDT
(In reply to comment #19)
> (In reply to comment #18)
> > Something wrong with test (or the API). It is not passing since it landed.
> 
> I'm guessing because the dependency (Bug 94248) has not landed?

Indeed. It landed now and tests are passing. Would be nice next time land the utest together or right after the patch it depends on.