Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1864713003:
[MD settings] appearance browser tests (Closed)
|
Created:
4 years, 8 months ago by dschuyler Modified:
4 years, 8 months ago Reviewers:
dpapad CC:
arv+watch_chromium.org, chromium-reviews, dbeam+watch-settings_chromium.org, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, stevenjb+watch-md-settings_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[MD settings] appearance browser tests
This CL adds a simple appearance browser test for the font data.
The old non-browser proxy tests are also being removed.
BUG=531786
Committed: https://crrev.com/1ab2cbeafaf9e2573a91e21fbe271bab3cf9484c
Cr-Commit-Position: refs/heads/master@{#387120}
Patch Set 1 : cleanup #
Total comments: 8
Patch Set 2 : review changes #
Total comments: 2
Patch Set 3 : review nit #Patch Set 4 : changing test file name #Patch Set 5 : fix test proxy #
Total comments: 3
Messages
Total messages: 26 (10 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
dschuyler@chromium.org changed reviewers: + dpapad@chromium.org
This CL has only one real test, intending to focus on the test setup (framework).
https://codereview.chromium.org/1864713003/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/settings/appearance_browsertest.js (right): https://codereview.chromium.org/1864713003/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/appearance_browsertest.js:24: fontsData: { Since fontsData is not a function, it makes more sense to declare it in the constructor instead of placing it on the prototype, for example see https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/data/w.... https://codereview.chromium.org/1864713003/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/appearance_browsertest.js:29: /** /** @override */ https://codereview.chromium.org/1864713003/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/appearance_browsertest.js:38: var fontsEntry = null; Nit: s/fontsEntry/fontsPage or simply page? https://codereview.chromium.org/1864713003/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/appearance_browsertest.js:58: var responsePromise = fontsBrowserProxy.fetchFontsData().then( |fetchPromise| tests that the actual production code is making a call to the browser proxy, so it is useful, but I am confused by the existence of |responsePromise|. It seems that it is testing the TestFontsBrwoserProxy itself. Could you explain what are you trying to test here? If you are simply trying to test that the appearance page makes a call to "fetchFontsData" on startup then the first promise should be enough. test('fetchFontsData', function() { return fontsBrowserProxy.whenCalled('fetchFontsData'); }); In addition to that I think it would make sense to test that |fontsEntry| is correctly populated according to the mock data you are providing, by making some assertions on the DOM structure. I don't see a reason the test should be calling |fetchFontsData| directly.
https://codereview.chromium.org/1864713003/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/settings/appearance_browsertest.js (right): https://codereview.chromium.org/1864713003/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/appearance_browsertest.js:24: fontsData: { On 2016/04/06 17:14:55, dpapad wrote: > Since fontsData is not a function, it makes more sense to declare it in the > constructor instead of placing it on the prototype, for example see > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/data/w.... Done. https://codereview.chromium.org/1864713003/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/appearance_browsertest.js:29: /** On 2016/04/06 17:14:55, dpapad wrote: > /** @override */ Done. https://codereview.chromium.org/1864713003/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/appearance_browsertest.js:38: var fontsEntry = null; On 2016/04/06 17:14:55, dpapad wrote: > Nit: s/fontsEntry/fontsPage or simply page? Done. https://codereview.chromium.org/1864713003/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/appearance_browsertest.js:58: var responsePromise = fontsBrowserProxy.fetchFontsData().then( On 2016/04/06 17:14:55, dpapad wrote: > |fetchPromise| tests that the actual production code is making a call to the > browser proxy, so it is useful, but I am confused by the existence of > |responsePromise|. It seems that it is testing the TestFontsBrwoserProxy itself. > Could you explain what are you trying to test here? > > If you are simply trying to test that the appearance page makes a call to > "fetchFontsData" on startup then the first promise should be enough. > > test('fetchFontsData', function() { > return fontsBrowserProxy.whenCalled('fetchFontsData'); > }); > > In addition to that I think it would make sense to test that |fontsEntry| is > correctly populated according to the mock data you are providing, by making some > assertions on the DOM structure. I don't see a reason the test should be calling > |fetchFontsData| directly. Done.
LGTM https://codereview.chromium.org/1864713003/diff/60001/chrome/test/data/webui/... File chrome/test/data/webui/settings/appearance_browsertest.js (right): https://codereview.chromium.org/1864713003/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/settings/appearance_browsertest.js:20: /** @private {!FontData} */ Typo? s/FontData/FontsData
Thanks! https://codereview.chromium.org/1864713003/diff/60001/chrome/test/data/webui/... File chrome/test/data/webui/settings/appearance_browsertest.js (right): https://codereview.chromium.org/1864713003/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/settings/appearance_browsertest.js:20: /** @private {!FontData} */ On 2016/04/07 21:24:26, dpapad wrote: > Typo? s/FontData/FontsData Done.
Offline, Demetrios suggested changing the file name on the test file. Done.
The CQ bit was checked by dschuyler@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpapad@chromium.org Link to the patchset: https://codereview.chromium.org/1864713003/#ps100001 (title: "changing test file name")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1864713003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1864713003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by dschuyler@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1864713003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1864713003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
fix for failing test bot.
lgtm https://codereview.chromium.org/1864713003/diff/120001/chrome/test/data/webui... File chrome/test/data/webui/settings/appearance_page_test.js (right): https://codereview.chromium.org/1864713003/diff/120001/chrome/test/data/webui... chrome/test/data/webui/settings/appearance_page_test.js:5: /** @fileoverview Runs polymer appearance font settings elements. */ Nit: /** @fileoverview Tests for appearance font settings elements. */ https://codereview.chromium.org/1864713003/diff/120001/chrome/test/data/webui... chrome/test/data/webui/settings/appearance_page_test.js:23: 'fontList': [['font name', 'alternate', 'ltr']], Nit(optional): Object property names do not need quotes, for example {hello: 'world'} is valid as is {'hello': 'world'}. https://codereview.chromium.org/1864713003/diff/120001/chrome/test/data/webui... File chrome/test/data/webui/settings/cr_settings_browsertest.js (right): https://codereview.chromium.org/1864713003/diff/120001/chrome/test/data/webui... chrome/test/data/webui/settings/cr_settings_browsertest.js:194: ROOT_PATH + 'ui/webui/resources/js/promise_resolver.js', Not needed anymore, since https://codereview.chromium.org/1886683002 has landed.
The CQ bit was checked by dschuyler@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1864713003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1864713003/120001
On 2016/04/13 19:06:25, dpapad wrote: > lgtm > > https://codereview.chromium.org/1864713003/diff/120001/chrome/test/data/webui... > File chrome/test/data/webui/settings/appearance_page_test.js (right): > > https://codereview.chromium.org/1864713003/diff/120001/chrome/test/data/webui... > chrome/test/data/webui/settings/appearance_page_test.js:5: /** @fileoverview > Runs polymer appearance font settings elements. */ > Nit: > /** @fileoverview Tests for appearance font settings elements. */ > > https://codereview.chromium.org/1864713003/diff/120001/chrome/test/data/webui... > chrome/test/data/webui/settings/appearance_page_test.js:23: 'fontList': [['font > name', 'alternate', 'ltr']], > Nit(optional): Object property names do not need quotes, for example > {hello: 'world'} is valid as is {'hello': 'world'}. > > https://codereview.chromium.org/1864713003/diff/120001/chrome/test/data/webui... > File chrome/test/data/webui/settings/cr_settings_browsertest.js (right): > > https://codereview.chromium.org/1864713003/diff/120001/chrome/test/data/webui... > chrome/test/data/webui/settings/cr_settings_browsertest.js:194: ROOT_PATH + > 'ui/webui/resources/js/promise_resolver.js', > Not needed anymore, since https://codereview.chromium.org/1886683002 has landed. These changes will be made soon in another CL.
Message was sent while issue was closed.
Committed patchset #5 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== [MD settings] appearance browser tests This CL adds a simple appearance browser test for the font data. The old non-browser proxy tests are also being removed. BUG=531786 ========== to ========== [MD settings] appearance browser tests This CL adds a simple appearance browser test for the font data. The old non-browser proxy tests are also being removed. BUG=531786 Committed: https://crrev.com/1ab2cbeafaf9e2573a91e21fbe271bab3cf9484c Cr-Commit-Position: refs/heads/master@{#387120} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/1ab2cbeafaf9e2573a91e21fbe271bab3cf9484c Cr-Commit-Position: refs/heads/master@{#387120} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
