Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2624003003:
WebUI: Add cr-policy-pref-indicator tests (Closed)
|
Created:
3 years, 11 months ago by stevenjb Modified:
3 years, 10 months ago CC:
chromium-reviews, michaelpg+watch-elements_chromium.org, stevenjb+watch-md-settings_chromium.org, dbeam+watch-elements_chromium.org, oshima+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionWebUI: Add cr-policy-pref-indicator tests
This adds some tests for cr-policy-pref-indicator.
It also fixes a potential issue if a pref property changes but not the entire pref.
BUG=679561
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2624003003
Cr-Commit-Position: refs/heads/master@{#451248}
Committed: https://chromium.googlesource.com/chromium/src/+/5f4f1ed5c5e0c32bda1b5da16f4f4dab37e276b8
Patch Set 1 #Patch Set 2 : Rebase #Patch Set 3 : Elim registerTests #
Total comments: 8
Patch Set 4 : Feedback #Patch Set 5 : Rebase + define required enums in suiteSetup() #Patch Set 6 : Reduce changes to cr-policy-pref-indicator #Patch Set 7 : Fix closure #
Total comments: 6
Patch Set 8 : Feedback #
Total comments: 4
Messages
Total messages: 31 (9 generated)
Description was changed from ========== WebUI: Fix cr-policy-pref-indicator and add tests This fixes some issues with cr-policy-pref-indicator when indicator-type is set directly, and potential issues if a pref property changes but not the entire pref. It also adds tests. BUG=679561 ========== to ========== WebUI: Fix cr-policy-pref-indicator and add tests This fixes some issues with cr-policy-pref-indicator when indicator-type is set directly, and potential issues if a pref property changes but not the entire pref. It also adds tests. BUG=679561 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
stevenjb@chromium.org changed reviewers: + dbeam@chromium.org
https://codereview.chromium.org/2624003003/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/cr_elements/cr_elements_browsertest.js (right): https://codereview.chromium.org/2624003003/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/cr_elements/cr_elements_browsertest.js:150: '../../../../../third_party/closure_compiler/externs/settings_private.js', it's really a huge bummer to execute externs https://codereview.chromium.org/2624003003/diff/40001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/policy/cr_policy_indicator_behavior.js (right): https://codereview.chromium.org/2624003003/diff/40001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/policy/cr_policy_indicator_behavior.js:55: icon = 'domain'; // Set a valid icon for accessability audit accessibility * https://codereview.chromium.org/2624003003/diff/40001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/policy/cr_policy_pref_indicator.js (right): https://codereview.chromium.org/2624003003/diff/40001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/policy/cr_policy_pref_indicator.js:39: prefChanged_() { prefChanged_: function
https://codereview.chromium.org/2624003003/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/cr_elements/cr_elements_browsertest.js (right): https://codereview.chromium.org/2624003003/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/cr_elements/cr_elements_browsertest.js:150: '../../../../../third_party/closure_compiler/externs/settings_private.js', On 2017/01/18 01:06:34, Dan Beam wrote: > it's really a huge bummer to execute externs don't we have a thing called "interfaces" for this instead?
https://codereview.chromium.org/2624003003/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/cr_elements/cr_elements_browsertest.js (right): https://codereview.chromium.org/2624003003/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/cr_elements/cr_elements_browsertest.js:150: '../../../../../third_party/closure_compiler/externs/settings_private.js', On 2017/01/18 01:06:52, Dan Beam (slow - converging) wrote: > On 2017/01/18 01:06:34, Dan Beam wrote: > > it's really a huge bummer to execute externs > > don't we have a thing called "interfaces" for this instead? *_interface.js explicitly can not be executed (and does not contain types). It contains a prototype for the interface and types for events. The only alternative I can think of would be to create something like *_private_types.js in this directory for the tests (copy/pasting types from the externs file). I am open to suggestions. https://codereview.chromium.org/2624003003/diff/40001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/policy/cr_policy_indicator_behavior.js (right): https://codereview.chromium.org/2624003003/diff/40001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/policy/cr_policy_indicator_behavior.js:55: icon = 'domain'; // Set a valid icon for accessability audit On 2017/01/18 01:06:34, Dan Beam (slow - converging) wrote: > accessibility * Done. https://codereview.chromium.org/2624003003/diff/40001/ui/webui/resources/cr_e... File ui/webui/resources/cr_elements/policy/cr_policy_pref_indicator.js (right): https://codereview.chromium.org/2624003003/diff/40001/ui/webui/resources/cr_e... ui/webui/resources/cr_elements/policy/cr_policy_pref_indicator.js:39: prefChanged_() { On 2017/01/18 01:06:34, Dan Beam (slow - converging) wrote: > prefChanged_: function Sigh. Sorry. Done.
so can we separate the tests and the fix? can we just add run the extension API for this URL or for this test? I swear I published a CL message somewhere, but maybe we could make a C++ test fixture that gives access to the extension API for this test rather than running externs or duplicating stuff?
Description was changed from ========== WebUI: Fix cr-policy-pref-indicator and add tests This fixes some issues with cr-policy-pref-indicator when indicator-type is set directly, and potential issues if a pref property changes but not the entire pref. It also adds tests. BUG=679561 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== WebUI: Add cr-policy-pref-indicator tests This adds some tests for cr-policy-pref-indicator. It also fixes a potential issue if a pref property changes but not the entire pref. BUG=679561 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Simplified the element changes significantly and eliminated a non-use case (setting indicator-type directly). If we need/want that at some point we can change that separately and add a test then. Also, after chatting with Devlin, we decided that the simplest solution by far is just to paste the 3 enums we need into suiteSetup(). LMKWYT.
stevenjb@chromium.org changed reviewers: + michaelpg@chromium.org
https://codereview.chromium.org/2624003003/diff/120001/chrome/test/data/webui... File chrome/test/data/webui/cr_elements/cr_policy_pref_indicator_tests.js (right): https://codereview.chromium.org/2624003003/diff/120001/chrome/test/data/webui... chrome/test/data/webui/cr_elements/cr_policy_pref_indicator_tests.js:52: nit: remove blank line https://codereview.chromium.org/2624003003/diff/120001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/policy/cr_policy_indicator_behavior.js (right): https://codereview.chromium.org/2624003003/diff/120001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/policy/cr_policy_indicator_behavior.js:55: icon = 'domain'; // Set a valid icon for accessibility audit why does the accessibility audit care if the element is hidden (which it should be in this case, I thought)? seems like having a real icon here could contribute to bugs (or accidentally correct behavior) in the future https://codereview.chromium.org/2624003003/diff/120001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/policy/cr_policy_pref_indicator.js (right): https://codereview.chromium.org/2624003003/diff/120001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/policy/cr_policy_pref_indicator.js:24: * @type {CrPolicyIndicatorType} @private?
https://codereview.chromium.org/2624003003/diff/120001/chrome/test/data/webui... File chrome/test/data/webui/cr_elements/cr_policy_pref_indicator_tests.js (right): https://codereview.chromium.org/2624003003/diff/120001/chrome/test/data/webui... chrome/test/data/webui/cr_elements/cr_policy_pref_indicator_tests.js:52: On 2017/02/15 02:02:11, michaelpg wrote: > nit: remove blank line Done. https://codereview.chromium.org/2624003003/diff/120001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/policy/cr_policy_indicator_behavior.js (right): https://codereview.chromium.org/2624003003/diff/120001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/policy/cr_policy_indicator_behavior.js:55: icon = 'domain'; // Set a valid icon for accessibility audit On 2017/02/15 02:02:11, michaelpg wrote: > why does the accessibility audit care if the element is hidden (which it should > be in this case, I thought)? > > seems like having a real icon here could contribute to bugs (or accidentally > correct behavior) in the future Turns out we don't need this any more, so nevermind. https://codereview.chromium.org/2624003003/diff/120001/ui/webui/resources/cr_... File ui/webui/resources/cr_elements/policy/cr_policy_pref_indicator.js (right): https://codereview.chromium.org/2624003003/diff/120001/ui/webui/resources/cr_... ui/webui/resources/cr_elements/policy/cr_policy_pref_indicator.js:24: * @type {CrPolicyIndicatorType} On 2017/02/15 02:02:11, michaelpg wrote: > @private? Done.
On 2017/02/15 00:08:30, stevenjb wrote: > Simplified the element changes significantly and eliminated a non-use case > (setting indicator-type directly). If we need/want that at some point we can > change that separately and add a test then. > > Also, after chatting with Devlin, we decided that the simplest solution by far > is just to paste the 3 enums we need into suiteSetup(). LMKWYT. Can you remind me what the issue was with importing the settings_private externs?
On 2017/02/16 04:23:50, michaelpg wrote: > On 2017/02/15 00:08:30, stevenjb wrote: > > Simplified the element changes significantly and eliminated a non-use case > > (setting indicator-type directly). If we need/want that at some point we can > > change that separately and add a test then. > > > > Also, after chatting with Devlin, we decided that the simplest solution by far > > is just to paste the 3 enums we need into suiteSetup(). LMKWYT. > > Can you remind me what the issue was with importing the settings_private > externs? Ah, there was this, from dbeam@: > https://codereview.chromium.org/2624003003/diff/40001/chrome/test/data/webui/... > chrome/test/data/webui/cr_elements/cr_elements_browsertest.js:150: > '../../../../../third_party/closure_compiler/externs/settings_private.js', > it's really a huge bummer to execute externs It would be less error-prone (someone changes the API, doesn't update the settings JS, the test still passes). I agree it's a bummer :-(
On 2017/02/16 04:26:08, michaelpg wrote: > On 2017/02/16 04:23:50, michaelpg wrote: > > On 2017/02/15 00:08:30, stevenjb wrote: > > > Simplified the element changes significantly and eliminated a non-use case > > > (setting indicator-type directly). If we need/want that at some point we can > > > change that separately and add a test then. > > > > > > Also, after chatting with Devlin, we decided that the simplest solution by > far > > > is just to paste the 3 enums we need into suiteSetup(). LMKWYT. > > > > Can you remind me what the issue was with importing the settings_private > > externs? > > Ah, there was this, from dbeam@: > > > > https://codereview.chromium.org/2624003003/diff/40001/chrome/test/data/webui/... > > chrome/test/data/webui/cr_elements/cr_elements_browsertest.js:150: > > '../../../../../third_party/closure_compiler/externs/settings_private.js', > > it's really a huge bummer to execute externs > > It would be less error-prone (someone changes the API, doesn't update the > settings JS, the test still passes). I agree it's a bummer :-( These are just enums and they are just strings. If the API changed the code would change and any meaningful test -would- fail, so the only actual risk here is that a test becomes unmeaningful, but that is always a risk with JS based tests since the language is so permissive.
On 2017/02/16 17:36:01, stevenjb wrote: > On 2017/02/16 04:26:08, michaelpg wrote: > > On 2017/02/16 04:23:50, michaelpg wrote: > > > On 2017/02/15 00:08:30, stevenjb wrote: > > > > Simplified the element changes significantly and eliminated a non-use case > > > > (setting indicator-type directly). If we need/want that at some point we > can > > > > change that separately and add a test then. > > > > > > > > Also, after chatting with Devlin, we decided that the simplest solution by > > far > > > > is just to paste the 3 enums we need into suiteSetup(). LMKWYT. > > > > > > Can you remind me what the issue was with importing the settings_private > > > externs? > > > > Ah, there was this, from dbeam@: > > > > > > > > https://codereview.chromium.org/2624003003/diff/40001/chrome/test/data/webui/... > > > chrome/test/data/webui/cr_elements/cr_elements_browsertest.js:150: > > > '../../../../../third_party/closure_compiler/externs/settings_private.js', > > > it's really a huge bummer to execute externs > > > > It would be less error-prone (someone changes the API, doesn't update the > > settings JS, the test still passes). I agree it's a bummer :-( > > These are just enums and they are just strings. If the API changed the code > would change and any meaningful test -would- fail, so the only actual risk here > is that a test becomes unmeaningful, but that is always a risk with JS based > tests since the language is so permissive. I suppose if the code didn't change (referenced outdated enums) the closure compile would fail, so that helps.
lgtm https://codereview.chromium.org/2624003003/diff/140001/chrome/test/data/webui... File chrome/test/data/webui/cr_elements/cr_policy_pref_indicator_tests.js (right): https://codereview.chromium.org/2624003003/diff/140001/chrome/test/data/webui... chrome/test/data/webui/cr_elements/cr_policy_pref_indicator_tests.js:5: /** @fileoverview Suite of tests for cr_policy-pref-indicator. */ cr-policy https://codereview.chromium.org/2624003003/diff/140001/chrome/test/data/webui... chrome/test/data/webui/cr_elements/cr_policy_pref_indicator_tests.js:44: // Set up strings used by policy indicator elements. Given the comment about settings_private.idl, could we make it clearer that this comes from elsewhere?
https://codereview.chromium.org/2624003003/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/cr_elements/cr_elements_browsertest.js (right): https://codereview.chromium.org/2624003003/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/cr_elements/cr_elements_browsertest.js:150: '../../../../../third_party/closure_compiler/externs/settings_private.js', On 2017/01/18 23:13:03, stevenjb wrote: > On 2017/01/18 01:06:52, Dan Beam (slow - converging) wrote: > > On 2017/01/18 01:06:34, Dan Beam wrote: > > > it's really a huge bummer to execute externs > > > > don't we have a thing called "interfaces" for this instead? > > *_interface.js explicitly can not be executed (and does not contain types). It > contains a prototype for the interface and types for events. > > The only alternative I can think of would be to create something like > *_private_types.js in this directory for the tests (copy/pasting types from the > externs file). I am open to suggestions. not lgtm
ah, sorry, didn't see you're not using externs any more lgtm
https://codereview.chromium.org/2624003003/diff/140001/chrome/test/data/webui... File chrome/test/data/webui/cr_elements/cr_policy_pref_indicator_tests.js (right): https://codereview.chromium.org/2624003003/diff/140001/chrome/test/data/webui... chrome/test/data/webui/cr_elements/cr_policy_pref_indicator_tests.js:5: /** @fileoverview Suite of tests for cr_policy-pref-indicator. */ On 2017/02/16 21:36:22, michaelpg wrote: > cr-policy Done. https://codereview.chromium.org/2624003003/diff/140001/chrome/test/data/webui... chrome/test/data/webui/cr_elements/cr_policy_pref_indicator_tests.js:44: // Set up strings used by policy indicator elements. On 2017/02/16 21:36:22, michaelpg wrote: > Given the comment about settings_private.idl, could we make it clearer that this > comes from elsewhere? Done.
The CQ bit was checked by stevenjb@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
who wants to volunteer to port these? :-P https://cs.chromium.org/chromium/src/chrome/browser/policy/policy_prefs_brows...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by michaelpg@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1487301682303930,
"parent_rev": "056c8fa959aa437e99d8a7f4ad1d717bebc6c81a", "commit_rev":
"5f4f1ed5c5e0c32bda1b5da16f4f4dab37e276b8"}
Message was sent while issue was closed.
Description was changed from ========== WebUI: Add cr-policy-pref-indicator tests This adds some tests for cr-policy-pref-indicator. It also fixes a potential issue if a pref property changes but not the entire pref. BUG=679561 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== WebUI: Add cr-policy-pref-indicator tests This adds some tests for cr-policy-pref-indicator. It also fixes a potential issue if a pref property changes but not the entire pref. BUG=679561 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2624003003 Cr-Commit-Position: refs/heads/master@{#451248} Committed: https://chromium.googlesource.com/chromium/src/+/5f4f1ed5c5e0c32bda1b5da16f4f... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/5f4f1ed5c5e0c32bda1b5da16f4f... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
