Bug 92617

Summary:

[EFL][wk2] Add unit tests for back-forward list API

Product:

WebKit

Reporter:

Mikhail Pozdnyakov <mikhail.pozdnyakov>

Component:

WebKit EFL

Assignee:

Mikhail Pozdnyakov <mikhail.pozdnyakov>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

cdumez, dglazkov, gyuyoung.kim, kenneth, lucas.de.marchi, rakuco, tmpsantos, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

92345, 94248

Bug Blocks:

61838, 90451

Attachments:

Description	Flags
patch	none
patch v2	kenneth: review+, webkit.review.bot: commit-queue-
Archive of layout-test-results from gce-cr-linux-07	none
to be landed	none

Mikhail Pozdnyakov

Reported 2012-07-30 01:02:23 PDT

Unit tests for back-forward list API should be added.

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
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Thiago Marcos P. Santos

Comment 1 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?

Chris Dumez

Comment 2 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.

Thiago Marcos P. Santos

Comment 3 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.

Mikhail Pozdnyakov

Comment 4 2012-08-16 14:07:59 PDT

Created attachment 158895 [details] patch Returning of technical debt :-)

Chris Dumez

Comment 5 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)?

Kenneth Rohde Christiansen

Comment 6 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

Chris Dumez

Comment 7 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) "

Mikhail Pozdnyakov

Comment 8 2012-08-17 00:16:31 PDT

Created attachment 159021 [details] patch v2 Chris, Kenneth thanks for review

Chris Dumez

Comment 9 2012-08-17 00:19:53 PDT

Comment on attachment 159021 [details] patch v2 LGTM. Thanks.

WebKit Review Bot

Comment 10 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

WebKit Review Bot

Comment 11 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

Thiago Marcos P. Santos

Comment 12 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?

Mikhail Pozdnyakov

Comment 13 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.

Mikhail Pozdnyakov

Comment 14 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.

Mikhail Pozdnyakov

Comment 15 2012-08-17 07:11:40 PDT

Created attachment 159120 [details] to be landed Retry.

WebKit Review Bot

Comment 16 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>

WebKit Review Bot

Comment 17 2012-08-17 08:37:43 PDT

All reviewed patches have been landed. Closing bug.

Thiago Marcos P. Santos

Comment 18 2012-08-17 14:36:23 PDT

Something wrong with test (or the API). It is not passing since it landed.

Chris Dumez

Comment 19 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?

Thiago Marcos P. Santos

Comment 20 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.

Note You need to log in before you can comment on or make changes to this bug.