Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2907763004:
js_checker.py: Replace custom GetElementByIdCheck with ESLint no-restricted-properties. (Closed)
Descriptionjs_checker.py: Replace custom GetElementByIdCheck with ESLint no-restricted-properties.
BUG=720034
Review-Url: https://codereview.chromium.org/2907763004
Cr-Commit-Position: refs/heads/master@{#475985}
Committed: https://chromium.googlesource.com/chromium/src/+/2674f928e4c18a87bb48941bd026af17713013d4
Patch Set 1 #
Total comments: 6
Patch Set 2 : Resolve conflicts. #
Messages
Total messages: 27 (13 generated)
The CQ bit was checked by dpapad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
dpapad@chromium.org changed reviewers: + dbeam@chromium.org
Example error message:
/chromium1/src/chrome/browser/resources/settings/direction_delegate.js
27:14 error 'document.getElementById' is restricted from being used. Use
$('id') or getSVGElement('id') from chrome://resources/js/util.js instead of
document.getElementById('id') no-restricted-properties
✖ 1 problem (1 error, 0 warnings)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2907763004/diff/1/tools/web_dev_style/js_chec... File tools/web_dev_style/js_checker_test.py (left): https://codereview.chromium.org/2907763004/diff/1/tools/web_dev_style/js_chec... tools/web_dev_style/js_checker_test.py:206: def ShouldFailGetElementByIdCheck(self, line): can we leave some of these?
https://codereview.chromium.org/2907763004/diff/1/tools/web_dev_style/js_chec... File tools/web_dev_style/js_checker_test.py (left): https://codereview.chromium.org/2907763004/diff/1/tools/web_dev_style/js_chec... tools/web_dev_style/js_checker_test.py:206: def ShouldFailGetElementByIdCheck(self, line): On 2017/05/30 at 16:17:26, Dan Beam wrote: > can we leave some of these? We would need to modify those tests to run self.checker.RunEsLintChecks(), which accepts files, not lines of text. Which means we would need to keep some test files around to pass them to the test. Do you think is valuable to maintain our own tests to cover ESLint's potential regressions? It seems unnecessary to me, given that we are (and will be) on stable version's of ESLint.
https://codereview.chromium.org/2907763004/diff/1/tools/web_dev_style/js_chec... File tools/web_dev_style/js_checker_test.py (left): https://codereview.chromium.org/2907763004/diff/1/tools/web_dev_style/js_chec... tools/web_dev_style/js_checker_test.py:206: def ShouldFailGetElementByIdCheck(self, line): On 2017/05/30 17:08:24, dpapad wrote: > On 2017/05/30 at 16:17:26, Dan Beam wrote: > > can we leave some of these? > > We would need to modify those tests to run self.checker.RunEsLintChecks(), which > accepts files, not lines of text. Which means we would need to keep some test > files around to pass them to the test. > > Do you think is valuable to maintain our own tests to cover ESLint's potential > regressions? It seems unnecessary to me, given that we are (and will be) on > stable version's of ESLint. yes
lgtm
https://codereview.chromium.org/2907763004/diff/1/tools/web_dev_style/js_chec... File tools/web_dev_style/js_checker_test.py (left): https://codereview.chromium.org/2907763004/diff/1/tools/web_dev_style/js_chec... tools/web_dev_style/js_checker_test.py:206: def ShouldFailGetElementByIdCheck(self, line): > > Do you think is valuable to maintain our own tests to cover ESLint's potential > > regressions? It seems unnecessary to me, given that we are (and will be) on > > stable version's of ESLint. > > yes Trying to address this comment, but I keep hitting obstacles. Specifically, our python tests (specifically SuperMoxTestBase) seem to run in a very restricted environment where most things have been replaced by mock objects. For example in order to run ESLint we need to use subprocess.Popen at [1], but this is restricted at [2]. It seems that SuperMoxTestBase tests are not meant to spawn other processes. [1] https://cs.chromium.org/chromium/src/third_party/node/node.py?q=third_party/n... [2] https://cs.chromium.org/chromium/tools/depot_tools/testing_support/super_mox....
https://codereview.chromium.org/2907763004/diff/1/tools/web_dev_style/js_chec... File tools/web_dev_style/js_checker_test.py (left): https://codereview.chromium.org/2907763004/diff/1/tools/web_dev_style/js_chec... tools/web_dev_style/js_checker_test.py:206: def ShouldFailGetElementByIdCheck(self, line): On 2017/05/30 18:30:15, dpapad wrote: > > > Do you think is valuable to maintain our own tests to cover ESLint's > potential > > > regressions? It seems unnecessary to me, given that we are (and will be) on > > > stable version's of ESLint. > > > > yes > > Trying to address this comment, but I keep hitting obstacles. Specifically, our > python tests (specifically SuperMoxTestBase) seem to run in a very restricted > environment where most things have been replaced by mock objects. For example in > order to run ESLint we need to use subprocess.Popen at [1], but this is > restricted at [2]. It seems that SuperMoxTestBase tests are not meant to spawn > other processes. > > [1] > https://cs.chromium.org/chromium/src/third_party/node/node.py?q=third_party/n... > [2] > https://cs.chromium.org/chromium/tools/depot_tools/testing_support/super_mox.... i think we should only use SuperMoxTestBase if it's helping us.
https://codereview.chromium.org/2907763004/diff/1/tools/web_dev_style/js_chec... File tools/web_dev_style/js_checker_test.py (left): https://codereview.chromium.org/2907763004/diff/1/tools/web_dev_style/js_chec... tools/web_dev_style/js_checker_test.py:206: def ShouldFailGetElementByIdCheck(self, line): On 2017/05/30 at 20:33:30, Dan Beam wrote: > On 2017/05/30 18:30:15, dpapad wrote: > > > > Do you think is valuable to maintain our own tests to cover ESLint's > > potential > > > > regressions? It seems unnecessary to me, given that we are (and will be) on > > > > stable version's of ESLint. > > > > > > yes > > > > Trying to address this comment, but I keep hitting obstacles. Specifically, our > > python tests (specifically SuperMoxTestBase) seem to run in a very restricted > > environment where most things have been replaced by mock objects. For example in > > order to run ESLint we need to use subprocess.Popen at [1], but this is > > restricted at [2]. It seems that SuperMoxTestBase tests are not meant to spawn > > other processes. > > > > [1] > > https://cs.chromium.org/chromium/src/third_party/node/node.py?q=third_party/n... > > [2] > > https://cs.chromium.org/chromium/tools/depot_tools/testing_support/super_mox.... > > i think we should only use SuperMoxTestBase if it's helping us. I am restoring similar tests at https://codereview.chromium.org/2917473002.
https://codereview.chromium.org/2907763004/diff/1/tools/web_dev_style/js_chec... File tools/web_dev_style/js_checker_test.py (left): https://codereview.chromium.org/2907763004/diff/1/tools/web_dev_style/js_chec... tools/web_dev_style/js_checker_test.py:206: def ShouldFailGetElementByIdCheck(self, line): On 2017/05/30 at 20:33:30, Dan Beam wrote: > On 2017/05/30 18:30:15, dpapad wrote: > > > > Do you think is valuable to maintain our own tests to cover ESLint's > > potential > > > > regressions? It seems unnecessary to me, given that we are (and will be) on > > > > stable version's of ESLint. > > > > > > yes > > > > Trying to address this comment, but I keep hitting obstacles. Specifically, our > > python tests (specifically SuperMoxTestBase) seem to run in a very restricted > > environment where most things have been replaced by mock objects. For example in > > order to run ESLint we need to use subprocess.Popen at [1], but this is > > restricted at [2]. It seems that SuperMoxTestBase tests are not meant to spawn > > other processes. > > > > [1] > > https://cs.chromium.org/chromium/src/third_party/node/node.py?q=third_party/n... > > [2] > > https://cs.chromium.org/chromium/tools/depot_tools/testing_support/super_mox.... > > i think we should only use SuperMoxTestBase if it's helping us. I am restoring similar tests at https://codereview.chromium.org/2917473002.
The CQ bit was checked by dpapad@chromium.org
The CQ bit was unchecked by dpapad@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by dpapad@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/2907763004/#ps20001 (title: "Resolve conflicts.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
The CQ bit was checked by dpapad@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": 20001, "attempt_start_ts": 1496252306532210,
"parent_rev": "3562f1b7410b884e07bbd2fc75cfee7e96ed2a41", "commit_rev":
"2674f928e4c18a87bb48941bd026af17713013d4"}
Message was sent while issue was closed.
Description was changed from ========== js_checker.py: Replace custom GetElementByIdCheck with ESLint no-restricted-properties. BUG=720034 ========== to ========== js_checker.py: Replace custom GetElementByIdCheck with ESLint no-restricted-properties. BUG=720034 Review-Url: https://codereview.chromium.org/2907763004 Cr-Commit-Position: refs/heads/master@{#475985} Committed: https://chromium.googlesource.com/chromium/src/+/2674f928e4c18a87bb48941bd026... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/2674f928e4c18a87bb48941bd026... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
