Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2078903002:
Set custom-scoped patterns in HostContentSettingsMapTest to keep test coverage (Closed)
|
Created:
4 years, 6 months ago by lshang Modified:
4 years, 5 months ago CC:
chromium-reviews, markusheintz_, msramek+watch_chromium.org, raymes+watch_chromium.org, chrome-apps-syd-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSet custom-scoped patterns in HostContentSettingsMapTest to keep test coverage
In https://codereview.chromium.org/1686343002 we removed patterns from some
tests in HostContentSettingsMapTest because we recommend developers to use
SetContentSettingDefaultScope to get appropriate domain-scoped/origin-scoped
patterns for different types.
But now since we are migrating all content settings types to be origin scoped,
we should set custom-scoped patterns in these tests so that they still could
test patterns and keep test coverage.
BUG=604612
Committed: https://crrev.com/7951c946d4b98ce8dd12718c66809fbf42ab6d54
Cr-Commit-Position: refs/heads/master@{#402383}
Patch Set 1 #
Total comments: 5
Patch Set 2 : enhance tests #
Total comments: 14
Patch Set 3 : adjust tests #
Total comments: 6
Patch Set 4 : address review comments #Messages
Total messages: 19 (7 generated)
Description was changed from ========== Set custome-scoped patterns in HostContentSettingsMapTest to keep test coverage change BUG= ========== to ========== Set custome-scoped patterns in HostContentSettingsMapTest to keep test coverage In https://codereview.chromium.org/1686343002 we removed patterns from some tests in HostContentSettingsMapTest because we recommend developers to use SetContentSettingDefaultScope to get appropriate domain-scoped/origin-scoped patterns for different types. But now since we are migrating all content settings types to be origin scoped, we should set custome-scoped patterns in these tests so that they still could test patterns and keep test coverage. BUG=604612 ==========
lshang@chromium.org changed reviewers: + msramek@chromium.org, raymes@chromium.org
PTAL thanks!
https://codereview.chromium.org/2078903002/diff/1/chrome/browser/content_sett... File chrome/browser/content_settings/host_content_settings_map_unittest.cc (right): https://codereview.chromium.org/2078903002/diff/1/chrome/browser/content_sett... chrome/browser/content_settings/host_content_settings_map_unittest.cc:578: TEST_F(HostContentSettingsMapTest, NestedSettings) { As discussed, this test is a bit confusing so maybe we should rewrite it. I also wonder whether we should have a test that ensures that changing a setting for one origin doesn't affect other origins (or subdomains).
Thanks for looking into this again :) I added some suggestions for another configurations to test. https://codereview.chromium.org/2078903002/diff/1/chrome/browser/content_sett... File chrome/browser/content_settings/host_content_settings_map_unittest.cc (right): https://codereview.chromium.org/2078903002/diff/1/chrome/browser/content_sett... chrome/browser/content_settings/host_content_settings_map_unittest.cc:578: TEST_F(HostContentSettingsMapTest, NestedSettings) { On 2016/06/20 04:43:59, raymes wrote: > As discussed, this test is a bit confusing so maybe we should rewrite it. > > I also wonder whether we should have a test that ensures that changing a setting > for one origin doesn't affect other origins (or subdomains). Agreed. We're currently testing that other types (GEOLOCATION in this case) are not affected, we should also test that other unrelated URLs and domains are not. We can also test that Pattern::FromString("a.b.example.com") does not affect "https://b.example.com", i.e. that the two are not equivalent, but one contains the other. Finally, since this is about the evaluation of nested testing, we can add a test to alternate allow and block for the same type: Set(COOKIES, "[*.]example.com", BLOCK) Set(COOKIES, "[*.]b.example.com", ALLOW) Set(COOKIES, "[*.]a.b.example.com", BLOCK) and then test "example.com", "b.example.com", "a.example.com", "a.b.example.com", "b.b.example.com" etc. https://codereview.chromium.org/2078903002/diff/1/chrome/browser/content_sett... chrome/browser/content_settings/host_content_settings_map_unittest.cc:606: EXPECT_EQ(CONTENT_SETTING_BLOCK, nit: For better readability, please reorder this section to POPUPS, COOKIES, AUTOMATIC_DOWNLOADS, JAVASCRIPT, so they're in the same order as above.
I added some more test cases. PTALA thanks! https://codereview.chromium.org/2078903002/diff/1/chrome/browser/content_sett... File chrome/browser/content_settings/host_content_settings_map_unittest.cc (right): https://codereview.chromium.org/2078903002/diff/1/chrome/browser/content_sett... chrome/browser/content_settings/host_content_settings_map_unittest.cc:578: TEST_F(HostContentSettingsMapTest, NestedSettings) { On 2016/06/20 16:20:04, msramek wrote: > On 2016/06/20 04:43:59, raymes wrote: > > As discussed, this test is a bit confusing so maybe we should rewrite it. > > > > I also wonder whether we should have a test that ensures that changing a > setting > > for one origin doesn't affect other origins (or subdomains). > > Agreed. > > We're currently testing that other types (GEOLOCATION in this case) are not > affected, we should also test that other unrelated URLs and domains are not. > > We can also test that Pattern::FromString("a.b.example.com") does not affect > "https://b.example.com", i.e. that the two are not equivalent, but one contains > the other. Just to clarify, did you mean Pattern::FromString("b.example.com") won't affect "https://a.b.example.com"? Pattern::FromString("a.b.example.com") doesn't contain "https://b.example.com" I think.:-) > > Finally, since this is about the evaluation of nested testing, we can add a test > to alternate allow and block for the same type: > Set(COOKIES, "[*.]example.com", BLOCK) > Set(COOKIES, "[*.]b.example.com", ALLOW) > Set(COOKIES, "[*.]a.b.example.com", BLOCK) > and then test "example.com", "b.example.com", "a.example.com", > "a.b.example.com", "b.b.example.com" etc. Done. https://codereview.chromium.org/2078903002/diff/1/chrome/browser/content_sett... chrome/browser/content_settings/host_content_settings_map_unittest.cc:606: EXPECT_EQ(CONTENT_SETTING_BLOCK, On 2016/06/20 16:20:04, msramek wrote: > nit: For better readability, please reorder this section to POPUPS, COOKIES, > AUTOMATIC_DOWNLOADS, JAVASCRIPT, so they're in the same order as above. Done. I deleted NOTIFICATIONS, FULLSCREEN, MOUSELOCK, KEYGEN and AUTOPLAY below since we already use GEOLOCATION to test that patterns don't affect other types, I think we don't need to list them all?
Thanks Liu https://codereview.chromium.org/2078903002/diff/20001/chrome/browser/content_... File chrome/browser/content_settings/host_content_settings_map_unittest.cc (right): https://codereview.chromium.org/2078903002/diff/20001/chrome/browser/content_... chrome/browser/content_settings/host_content_settings_map_unittest.cc:340: ContentSettingsPattern::FromString("example.com"); How about we use FromURLNoWildcard here (or SetDefaultScope on an origin-scoped setting) https://codereview.chromium.org/2078903002/diff/20001/chrome/browser/content_... chrome/browser/content_settings/host_content_settings_map_unittest.cc:350: // Changing a setting for one origin doesn't affect subdomains. Maybe move this (and the comment below) as comments for the test? https://codereview.chromium.org/2078903002/diff/20001/chrome/browser/content_... chrome/browser/content_settings/host_content_settings_map_unittest.cc:625: CONTENT_SETTINGS_TYPE_COOKIES, NULL)); nullptr https://codereview.chromium.org/2078903002/diff/20001/chrome/browser/content_... chrome/browser/content_settings/host_content_settings_map_unittest.cc:644: GURL host4("http://a.example.com/"); We should be consistent and either have all of these URLs defined at the top or close to where they are used. https://codereview.chromium.org/2078903002/diff/20001/chrome/browser/content_... chrome/browser/content_settings/host_content_settings_map_unittest.cc:657: CONTENT_SETTINGS_TYPE_COOKIES, NULL)); nullptr https://codereview.chromium.org/2078903002/diff/20001/chrome/browser/content_... chrome/browser/content_settings/host_content_settings_map_unittest.cc:686: // Nested setting of one type doesn't affect other types. I think we could move this (from here down) into a separate test function. See the comment below for more thoughts. https://codereview.chromium.org/2078903002/diff/20001/chrome/browser/content_... chrome/browser/content_settings/host_content_settings_map_unittest.cc:714: host3, host3, CONTENT_SETTINGS_TYPE_JAVASCRIPT, std::string())); It's not completely clear to me why we need to have all those types here. Couldn't we just set one pattern for one type and then check that geolocation still returns the correct value? Also I don't think it's important to specifically test that nested settings don't impact other types, so we could just call the test TypeSettingsAreIsolated or something (if you can think of a better name use that :)
https://codereview.chromium.org/2078903002/diff/20001/chrome/browser/content_... File chrome/browser/content_settings/host_content_settings_map_unittest.cc (right): https://codereview.chromium.org/2078903002/diff/20001/chrome/browser/content_... chrome/browser/content_settings/host_content_settings_map_unittest.cc:340: ContentSettingsPattern::FromString("example.com"); On 2016/06/23 01:15:19, raymes wrote: > How about we use FromURLNoWildcard here (or SetDefaultScope on an origin-scoped > setting) Done. https://codereview.chromium.org/2078903002/diff/20001/chrome/browser/content_... chrome/browser/content_settings/host_content_settings_map_unittest.cc:350: // Changing a setting for one origin doesn't affect subdomains. On 2016/06/23 01:15:19, raymes wrote: > Maybe move this (and the comment below) as comments for the test? Done. https://codereview.chromium.org/2078903002/diff/20001/chrome/browser/content_... chrome/browser/content_settings/host_content_settings_map_unittest.cc:625: CONTENT_SETTINGS_TYPE_COOKIES, NULL)); On 2016/06/23 01:15:18, raymes wrote: > nullptr Done. https://codereview.chromium.org/2078903002/diff/20001/chrome/browser/content_... chrome/browser/content_settings/host_content_settings_map_unittest.cc:644: GURL host4("http://a.example.com/"); On 2016/06/23 01:15:19, raymes wrote: > We should be consistent and either have all of these URLs defined at the top or > close to where they are used. Done. Move them all at the top. https://codereview.chromium.org/2078903002/diff/20001/chrome/browser/content_... chrome/browser/content_settings/host_content_settings_map_unittest.cc:657: CONTENT_SETTINGS_TYPE_COOKIES, NULL)); On 2016/06/23 01:15:19, raymes wrote: > nullptr Done. https://codereview.chromium.org/2078903002/diff/20001/chrome/browser/content_... chrome/browser/content_settings/host_content_settings_map_unittest.cc:686: // Nested setting of one type doesn't affect other types. On 2016/06/23 01:15:19, raymes wrote: > I think we could move this (from here down) into a separate test function. See > the comment below for more thoughts. Done. https://codereview.chromium.org/2078903002/diff/20001/chrome/browser/content_... chrome/browser/content_settings/host_content_settings_map_unittest.cc:714: host3, host3, CONTENT_SETTINGS_TYPE_JAVASCRIPT, std::string())); On 2016/06/23 01:15:19, raymes wrote: > It's not completely clear to me why we need to have all those types here. > Couldn't we just set one pattern for one type and then check that geolocation > still returns the correct value? > > Also I don't think it's important to specifically test that nested settings > don't impact other types, so we could just call the test TypeSettingsAreIsolated > or something (if you can think of a better name use that :) I agree.
LGTM (sorry for the delay!). https://codereview.chromium.org/2078903002/diff/40001/chrome/browser/content_... File chrome/browser/content_settings/host_content_settings_map_unittest.cc (right): https://codereview.chromium.org/2078903002/diff/40001/chrome/browser/content_... chrome/browser/content_settings/host_content_settings_map_unittest.cc:307: ContentSettingsPattern::FromString("example.org"); We should also have tests for crazier patterns such as "https://*", "http://www.google.com:*", but it doesn't have to be in this CL. (we're probably already well beyond your intended scope of this CL :) ) https://codereview.chromium.org/2078903002/diff/40001/chrome/browser/content_... chrome/browser/content_settings/host_content_settings_map_unittest.cc:339: GURL host3("http://example.org/"); Consider also adding a test for different port, e.g. "http://example.com:8080/". https://codereview.chromium.org/2078903002/diff/40001/chrome/browser/content_... chrome/browser/content_settings/host_content_settings_map_unittest.cc:658: GURL https_host2("https://b.example.com/"); nit: 1,2 instead of 2,3
Thanks Martin! https://codereview.chromium.org/2078903002/diff/40001/chrome/browser/content_... File chrome/browser/content_settings/host_content_settings_map_unittest.cc (right): https://codereview.chromium.org/2078903002/diff/40001/chrome/browser/content_... chrome/browser/content_settings/host_content_settings_map_unittest.cc:307: ContentSettingsPattern::FromString("example.org"); On 2016/06/24 19:53:48, msramek wrote: > We should also have tests for crazier patterns such as "https://*", > "http://www.google.com:*", but it doesn't have to be in this CL. (we're probably > already well beyond your intended scope of this CL :) ) Now I realized how we lack of full coverage of patterns. I would be happy to take care of this on a separate CL:-) https://codereview.chromium.org/2078903002/diff/40001/chrome/browser/content_... chrome/browser/content_settings/host_content_settings_map_unittest.cc:339: GURL host3("http://example.org/"); On 2016/06/24 19:53:48, msramek wrote: > Consider also adding a test for different port, e.g. "http://example.com:8080/". Done. https://codereview.chromium.org/2078903002/diff/40001/chrome/browser/content_... chrome/browser/content_settings/host_content_settings_map_unittest.cc:658: GURL https_host2("https://b.example.com/"); On 2016/06/24 19:53:48, msramek wrote: > nit: 1,2 instead of 2,3 Done.
lgtm
Description was changed from ========== Set custome-scoped patterns in HostContentSettingsMapTest to keep test coverage In https://codereview.chromium.org/1686343002 we removed patterns from some tests in HostContentSettingsMapTest because we recommend developers to use SetContentSettingDefaultScope to get appropriate domain-scoped/origin-scoped patterns for different types. But now since we are migrating all content settings types to be origin scoped, we should set custome-scoped patterns in these tests so that they still could test patterns and keep test coverage. BUG=604612 ========== to ========== Set custom-scoped patterns in HostContentSettingsMapTest to keep test coverage In https://codereview.chromium.org/1686343002 we removed patterns from some tests in HostContentSettingsMapTest because we recommend developers to use SetContentSettingDefaultScope to get appropriate domain-scoped/origin-scoped patterns for different types. But now since we are migrating all content settings types to be origin scoped, we should set custom-scoped patterns in these tests so that they still could test patterns and keep test coverage. BUG=604612 ==========
The CQ bit was checked by lshang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msramek@chromium.org Link to the patchset: https://codereview.chromium.org/2078903002/#ps60001 (title: "address review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Set custom-scoped patterns in HostContentSettingsMapTest to keep test coverage In https://codereview.chromium.org/1686343002 we removed patterns from some tests in HostContentSettingsMapTest because we recommend developers to use SetContentSettingDefaultScope to get appropriate domain-scoped/origin-scoped patterns for different types. But now since we are migrating all content settings types to be origin scoped, we should set custom-scoped patterns in these tests so that they still could test patterns and keep test coverage. BUG=604612 ========== to ========== Set custom-scoped patterns in HostContentSettingsMapTest to keep test coverage In https://codereview.chromium.org/1686343002 we removed patterns from some tests in HostContentSettingsMapTest because we recommend developers to use SetContentSettingDefaultScope to get appropriate domain-scoped/origin-scoped patterns for different types. But now since we are migrating all content settings types to be origin scoped, we should set custom-scoped patterns in these tests so that they still could test patterns and keep test coverage. BUG=604612 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Set custom-scoped patterns in HostContentSettingsMapTest to keep test coverage In https://codereview.chromium.org/1686343002 we removed patterns from some tests in HostContentSettingsMapTest because we recommend developers to use SetContentSettingDefaultScope to get appropriate domain-scoped/origin-scoped patterns for different types. But now since we are migrating all content settings types to be origin scoped, we should set custom-scoped patterns in these tests so that they still could test patterns and keep test coverage. BUG=604612 ========== to ========== Set custom-scoped patterns in HostContentSettingsMapTest to keep test coverage In https://codereview.chromium.org/1686343002 we removed patterns from some tests in HostContentSettingsMapTest because we recommend developers to use SetContentSettingDefaultScope to get appropriate domain-scoped/origin-scoped patterns for different types. But now since we are migrating all content settings types to be origin scoped, we should set custom-scoped patterns in these tests so that they still could test patterns and keep test coverage. BUG=604612 Committed: https://crrev.com/7951c946d4b98ce8dd12718c66809fbf42ab6d54 Cr-Commit-Position: refs/heads/master@{#402383} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/7951c946d4b98ce8dd12718c66809fbf42ab6d54 Cr-Commit-Position: refs/heads/master@{#402383} |
