Issue 2945563002: ChromeWebView integration test for Incognito.

Issue 2945563002: ChromeWebView integration test for Incognito. (Closed)

Created:
3 years, 6 months ago by Eugene But (OOO till 7-30)

Modified:
3 years, 6 months ago

Reviewers:
michaeldo, Hiroshi Ichikawa

CC:
chromium-reviews, Eugene But (OOO till 7-30), ios-reviews+web_chromium.org, ios-reviews_chromium.org

Target Ref:
refs/heads/master

Project:
chromium

Visibility:
Public.

More Reviews

Description

ChromeWebView integration tests for Incognito. These tests verify that browsing data (cookie, localStorage) does not leak between incognito and non-incognito web views. BUG=None Review-Url: https://codereview.chromium.org/2945563002 Cr-Commit-Position: refs/heads/master@{#481755} Committed: https://chromium.googlesource.com/chromium/src/+/aa87813942a324671466d574bc8567c0f74a7749

Patch Set 1 #

Total comments: 9

Patch Set 2 : Addressed review comments #

Patch Set 3 : Self review #

Created: 3 years, 6 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+134 lines, -3 lines)			Patch
M	ios/web_view/test/BUILD.gn	View		1 chunk	+1 line, -0 lines	0 comments	Download
A	ios/web_view/test/web_view_incognito_inttest.mm	View	1 2	1 chunk	+122 lines, -0 lines	0 comments	Download
M	ios/web_view/test/web_view_test_util.h	View		1 chunk	+5 lines, -0 lines	0 comments	Download
M	ios/web_view/test/web_view_test_util.mm	View		1 chunk	+6 lines, -3 lines	0 comments	Download

Messages

Total messages: 14 (6 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

Hiroshi Ichikawa

https://codereview.chromium.org/2945563002/diff/1/ios/web_view/test/web_view_incognito_inttest.mm File ios/web_view/test/web_view_incognito_inttest.mm (right): https://codereview.chromium.org/2945563002/diff/1/ios/web_view/test/web_view_incognito_inttest.mm#newcode32 ios/web_view/test/web_view_incognito_inttest.mm:32: TEST_F(WebViewIncognitoTest, BrowsingDataLeaking) { It can be TODO, but do ...

3 years, 6 months ago (2017-06-16 08:28:20 UTC) #3

michaeldo

lgtm I definitely think we should have this test, but I am slightly worried about ...

3 years, 6 months ago (2017-06-16 20:04:28 UTC) #4

Eugene But (OOO till 7-30)

> I definitely think we should have this test, but I am slightly worried about ...

3 years, 6 months ago (2017-06-20 00:36:32 UTC) #6

> I definitely think we should have this test, but I am slightly worried about
> potential flakiness. A similar ios/web test for Cookie partitioning has been
> disabled for being flaky for some time. (crbug.com/684024) Of course, that
test
> could be flaky for other reasons, but we should just be aware of it.
We don't really know why CookiePartitioning test is flaky, so let's just see how
this test behaves.

https://codereview.chromium.org/2945563002/diff/1/ios/web_view/test/web_view_...
File ios/web_view/test/web_view_incognito_inttest.mm (right):

https://codereview.chromium.org/2945563002/diff/1/ios/web_view/test/web_view_...
ios/web_view/test/web_view_incognito_inttest.mm:23: CWVWebView*
CreateIncognitoWebView() {
On 2017/06/16 20:04:27, michaeldo wrote:
> Is this helper necessary? I'm ok with it, but I don't think we need it since
you
> added the helper to take the configuration and you only call it once.
Now it is called 2 times :)

https://codereview.chromium.org/2945563002/diff/1/ios/web_view/test/web_view_...
ios/web_view/test/web_view_incognito_inttest.mm:32: TEST_F(WebViewIncognitoTest,
BrowsingDataLeaking) {
On 2017/06/16 08:28:20, Hiroshi Ichikawa wrote:
> It can be TODO, but do you want to check the opposite (incognito web view
> cookies not leaking to normal web view) too?
Added that test

https://codereview.chromium.org/2945563002/diff/1/ios/web_view/test/web_view_...
ios/web_view/test/web_view_incognito_inttest.mm:34: GURL url =
GetUrlForPageWithHtmlBody("<html><html>");
On 2017/06/16 20:04:27, michaeldo wrote:
> On 2017/06/16 08:28:20, Hiroshi Ichikawa wrote:
> > Do you mean <html></html>?
> 
> There is no need for the html tag here, you can pass an empty string. The
param
> string will be inserted inside the "body" tags of an html page.
> 
>
https://cs.chromium.org/chromium/src/ios/web_view/test/chrome_web_view_test.h...

Done.

https://codereview.chromium.org/2945563002/diff/1/ios/web_view/test/web_view_...
ios/web_view/test/web_view_incognito_inttest.mm:47: CWVWebView*
non_incognito_web_view =
On 2017/06/16 20:04:27, michaeldo wrote:
> Can this part be split into another test? Maybe "BrowsingDataShared"?
> // Tests that browsing data (cookie and localStorage) is shared between
> // two non-incognito web views.
This is more like a precondition, rather than actual test. Te test itself is not
very quick, so I'd rather keep this check here.

commit-bot: I haz the power

CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2945563002/40001

3 years, 6 months ago (2017-06-22 22:03:31 UTC) #10

commit-bot: I haz the power

Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/aa87813942a324671466d574bc8567c0f74a7749

3 years, 6 months ago (2017-06-23 00:59:53 UTC) #13

Eugene But (OOO till 7-30)

3 years, 5 months ago (2017-06-23 15:19:23 UTC) #14

Message was sent while issue was closed.

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/2952403003/ by eugenebut@chromium.org.

The reason for reverting is: Failing on downstream bots.

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages