MD Settings: Fix site data details regression and add test.

MD Settings: Fix site data details regression and add test.

BUG=689461
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2687553003
Cr-Commit-Position: refs/heads/master@{#449185}
Committed: https://chromium.googlesource.com/chromium/src/+/f2ad0c868e66c4692862ed29ff6d30a97440c0f0

[dpapad, 2017-02-07 23:37:37]

Description was changed from

==========
MD Settings: Fix site data details regression and add test.

BUG=689461
==========

to

==========
MD Settings: Fix site data details regression and add test.

BUG=689461
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[dpapad, 2017-02-07 23:47:03]

The CQ bit was checked by dpapad@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-07 23:48:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dpapad, 2017-02-08 00:04:09]

dpapad@chromium.org changed reviewers:
+ tommycli@chromium.org

[dpapad, 2017-02-08 00:04:10]

https://codereview.chromium.org/2687553003/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/cr_settings_browsertest.js (right):

https://codereview.chromium.org/2687553003/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/cr_settings_browsertest.js:680:
site_data_details.registerTests();
It is unfortunate that I have to use registerTests() here, but I could not find
a good way to avoid this. The new test I am adding is appending a "site=foo"
parameter to the URL, which affects the behavior of the site_details_tests.js
above. So essentially to preserve the old behavior of site_details_test.js, I
needed a way to either

 1 register the new test last, done via an explicit call to registerTests(), so
that the URL has not changed yet, by the time site_details_test.js runs, OR
 2 revert the URL change in the setup() of site_details_test.js.

I chose the 1st one.

[commit-bot: I haz the power, 2017-02-08 01:10:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-08 01:10:24]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[dpapad, 2017-02-08 19:21:02]

On 2017/02/08 at 00:04:10, dpapad wrote:
>
https://codereview.chromium.org/2687553003/diff/20001/chrome/test/data/webui/...
> File chrome/test/data/webui/settings/cr_settings_browsertest.js (right):
> 
>
https://codereview.chromium.org/2687553003/diff/20001/chrome/test/data/webui/...
> chrome/test/data/webui/settings/cr_settings_browsertest.js:680:
site_data_details.registerTests();
> It is unfortunate that I have to use registerTests() here, but I could not
find a good way to avoid this. The new test I am adding is appending a
"site=foo" parameter to the URL, which affects the behavior of the
site_details_tests.js above. So essentially to preserve the old behavior of
site_details_test.js, I needed a way to either
> 
>  1 register the new test last, done via an explicit call to registerTests(),
so that the URL has not changed yet, by the time site_details_test.js runs, OR
>  2 revert the URL change in the setup() of site_details_test.js.
> 
> I chose the 1st one.

Friendly ping.

[tommycli, 2017-02-08 19:28:40]

sorry, here's my review!

https://codereview.chromium.org/2687553003/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/cr_settings_browsertest.js (right):

https://codereview.chromium.org/2687553003/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/cr_settings_browsertest.js:680:
site_data_details.registerTests();
On 2017/02/08 00:04:10, dpapad wrote:
> It is unfortunate that I have to use registerTests() here, but I could not
find
> a good way to avoid this. The new test I am adding is appending a "site=foo"
> parameter to the URL, which affects the behavior of the site_details_tests.js
> above. So essentially to preserve the old behavior of site_details_test.js, I
> needed a way to either
> 
>  1 register the new test last, done via an explicit call to registerTests(),
so
> that the URL has not changed yet, by the time site_details_test.js runs, OR
>  2 revert the URL change in the setup() of site_details_test.js.
> 
> I chose the 1st one.

I probably would revert the URL change in the teardown of the test. Or -- if
that URL change is only applicable to that one test, do it in the test directly
(and revert in the test directly) instead of the setup/teardown methods.

I don't like to nitpick, but having this test be last seems to be fairly
fragile. If someone adds another test after this, it's going to mysteriously not
work without any obvious reason why.

I don't care if you leave it as a separate js file or combine it... that's up to
you.

https://codereview.chromium.org/2687553003/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/site_data_details_subpage_tests.js (right):

https://codereview.chromium.org/2687553003/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/site_data_details_subpage_tests.js:53: return
browserProxy.whenCalled('getCookieDetails').then(
This test does fail without the one char change right? I think it does, but I
wasn't completely sure from a quick read.

[dpapad, 2017-02-08 22:01:09]

Addressed comments. PTAL

https://codereview.chromium.org/2687553003/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/cr_settings_browsertest.js (right):

https://codereview.chromium.org/2687553003/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/cr_settings_browsertest.js:680:
site_data_details.registerTests();
On 2017/02/08 at 19:28:40, tommycli wrote:
> On 2017/02/08 00:04:10, dpapad wrote:
> > It is unfortunate that I have to use registerTests() here, but I could not
find
> > a good way to avoid this. The new test I am adding is appending a "site=foo"
> > parameter to the URL, which affects the behavior of the
site_details_tests.js
> > above. So essentially to preserve the old behavior of site_details_test.js,
I
> > needed a way to either
> > 
> >  1 register the new test last, done via an explicit call to registerTests(),
so
> > that the URL has not changed yet, by the time site_details_test.js runs, OR
> >  2 revert the URL change in the setup() of site_details_test.js.
> > 
> > I chose the 1st one.
> 
> I probably would revert the URL change in the teardown of the test. Or -- if
that URL change is only applicable to that one test, do it in the test directly
(and revert in the test directly) instead of the setup/teardown methods.
> 
> I don't like to nitpick, but having this test be last seems to be fairly
fragile. If someone adds another test after this, it's going to mysteriously not
work without any obvious reason why.
> 
> I don't care if you leave it as a separate js file or combine it... that's up
to you.

Done. Thanks for the help.

https://codereview.chromium.org/2687553003/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/site_data_details_subpage_tests.js (right):

https://codereview.chromium.org/2687553003/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/site_data_details_subpage_tests.js:53: return
browserProxy.whenCalled('getCookieDetails').then(
On 2017/02/08 at 19:28:40, tommycli wrote:
> This test does fail without the one char change right? I think it does, but I
wasn't completely sure from a quick read.

Yes, I doubled checked that. It fails with the following

AssertionError: expected 'abcd' to equal 'dummy_cookie_contents'
    at Function.assert.strictEqual (chai.js:2277:32)
    at assertEquals (test_api.js:893:17)
    at site_data_details_subpage_tests.js:73:13
    at site_data_details_subpage_tests.js:71:30
    at <anonymous>
", source: mocha_adapter.js (48)

[tommycli, 2017-02-08 22:04:17]

lgtm one nitpick. Thanks!

https://codereview.chromium.org/2687553003/diff/40001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/site_list_tests.js (right):

https://codereview.chromium.org/2687553003/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/settings/site_list_tests.js:349:
settings.navigateTo(settings.Route.BASIC);
well technically it's the internal route variable that matters, not the actual
URL...

[dpapad, 2017-02-08 22:16:43]

https://codereview.chromium.org/2687553003/diff/40001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/site_list_tests.js (right):

https://codereview.chromium.org/2687553003/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/settings/site_list_tests.js:349:
settings.navigateTo(settings.Route.BASIC);
On 2017/02/08 at 22:04:17, tommycli wrote:
> well technically it's the internal route variable that matters, not the actual
URL...

I see. In that case, how about calling resetRouteForTesting() instead (see new
patch)? (I had added that method in a previous CL, necessary for when tests run
in vulcanized mode).

[tommycli, 2017-02-08 22:28:13]

On 2017/02/08 22:16:43, dpapad wrote:
>
https://codereview.chromium.org/2687553003/diff/40001/chrome/test/data/webui/...
> File chrome/test/data/webui/settings/site_list_tests.js (right):
> 
>
https://codereview.chromium.org/2687553003/diff/40001/chrome/test/data/webui/...
> chrome/test/data/webui/settings/site_list_tests.js:349:
> settings.navigateTo(settings.Route.BASIC);
> On 2017/02/08 at 22:04:17, tommycli wrote:
> > well technically it's the internal route variable that matters, not the
actual
> URL...
> 
> I see. In that case, how about calling resetRouteForTesting() instead (see new
> patch)? (I had added that method in a previous CL, necessary for when tests
run
> in vulcanized mode).

Either sounds good. In this CL or the next. It sounds like we understand the
problem now. Still lgtm I leave the rest to your discretion.

[tommycli, 2017-02-08 22:35:24]

On 2017/02/08 22:28:13, tommycli wrote:
> On 2017/02/08 22:16:43, dpapad wrote:
> >
>
https://codereview.chromium.org/2687553003/diff/40001/chrome/test/data/webui/...
> > File chrome/test/data/webui/settings/site_list_tests.js (right):
> > 
> >
>
https://codereview.chromium.org/2687553003/diff/40001/chrome/test/data/webui/...
> > chrome/test/data/webui/settings/site_list_tests.js:349:
> > settings.navigateTo(settings.Route.BASIC);
> > On 2017/02/08 at 22:04:17, tommycli wrote:
> > > well technically it's the internal route variable that matters, not the
> actual
> > URL...
> > 
> > I see. In that case, how about calling resetRouteForTesting() instead (see
new
> > patch)? (I had added that method in a previous CL, necessary for when tests
> run
> > in vulcanized mode).
> 
> Either sounds good. In this CL or the next. It sounds like we understand the
> problem now. Still lgtm I leave the rest to your discretion.

Is there a base class we can use to always reset the route on teardown btw?

It seems that we would always want to do that. In a separate CL I mean.

[dpapad, 2017-02-08 23:05:16]

The CQ bit was checked by dpapad@chromium.org

[commit-bot: I haz the power, 2017-02-08 23:05:46]

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

[commit-bot: I haz the power, 2017-02-09 01:49:37]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1486595116192300,
"parent_rev": "3fbaee1204d803fba5c0f28e7502f57879546c62", "commit_rev":
"f2ad0c868e66c4692862ed29ff6d30a97440c0f0"}

[commit-bot: I haz the power, 2017-02-09 01:50:33]

Description was changed from

==========
MD Settings: Fix site data details regression and add test.

BUG=689461
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Fix site data details regression and add test.

BUG=689461
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2687553003
Cr-Commit-Position: refs/heads/master@{#449185}
Committed:
https://chromium.googlesource.com/chromium/src/+/f2ad0c868e66c4692862ed29ff6d...
==========

[commit-bot: I haz the power, 2017-02-09 01:50:34]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/f2ad0c868e66c4692862ed29ff6d...