js_checker.py: Restore smoke tests for linting violations.

js_checker.py: Restore smoke tests for linting violations.

Restoring test to ensure that document.getElementById() calls are flagged as
style violations, now that these checks are performed via ESLint. These tests
are not meant to be exhaustive, instead they just validate that our PRESUBMIT
integration with ESLint works.

BUG=720034
Review-Url: https://codereview.chromium.org/2917473002
Cr-Commit-Position: refs/heads/master@{#476014}
Committed: https://chromium.googlesource.com/chromium/src/+/5c9c24ec6d1d8ddaae83a9118c5824b6c40f9854

[dpapad, 2017-05-30 22:11:30]

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

[dpapad, 2017-05-30 22:11:31]

https://codereview.chromium.org/2917473002/diff/1/tools/web_dev_style/js_chec...
File tools/web_dev_style/js_checker.py (right):

https://codereview.chromium.org/2917473002/diff/1/tools/web_dev_style/js_chec...
tools/web_dev_style/js_checker.py:127: results +=
self.RunEsLintChecks(affected_js_files_paths)
I had to change the signature of RunEsLintChecks, to be able to directly call it
from tests. The problem is that even though I can setup a mock
input_api.AffectedFiles to return a MockFile instance, the MockFile instance is
useless to ESLint, it needs an actual file.

[Dan Beam, 2017-05-30 23:11:09]

https://codereview.chromium.org/2917473002/diff/1/tools/web_dev_style/js_chec...
File tools/web_dev_style/js_checker.py (right):

https://codereview.chromium.org/2917473002/diff/1/tools/web_dev_style/js_chec...
tools/web_dev_style/js_checker.py:127: results +=
self.RunEsLintChecks(affected_js_files_paths)
On 2017/05/30 22:11:30, dpapad wrote:
> I had to change the signature of RunEsLintChecks, to be able to directly call
it
> from tests. The problem is that even though I can setup a mock
> input_api.AffectedFiles to return a MockFile instance, the MockFile instance
is
> useless to ESLint, it needs an actual file.

wait, why doesn't this work?
https://cs.chromium.org/chromium/src/PRESUBMIT_test_mocks.py?sq=package:chrom...

https://codereview.chromium.org/2917473002/diff/60001/tools/web_dev_style/js_...
File tools/web_dev_style/js_checker_eslint_test.py (right):

https://codereview.chromium.org/2917473002/diff/60001/tools/web_dev_style/js_...
tools/web_dev_style/js_checker_eslint_test.py:13:
os.path.join(os.path.dirname(os.path.abspath(__file__)), '..', '..'))
indent is weird (should only be 4 \s for continuation)

https://codereview.chromium.org/2917473002/diff/60001/tools/web_dev_style/js_...
tools/web_dev_style/js_checker_eslint_test.py:15: from PRESUBMIT_test_mocks
import (MockInputApi, MockOutputApi)
nit: do you need the parens around the module names you're importing?

https://codereview.chromium.org/2917473002/diff/60001/tools/web_dev_style/js_...
tools/web_dev_style/js_checker_eslint_test.py:21: 
only \n (not \n\n) between these.

\n\n is only for top-level

https://codereview.chromium.org/2917473002/diff/60001/tools/web_dev_style/js_...
tools/web_dev_style/js_checker_eslint_test.py:24: os.remove(tmp_file)
arguable nit: map(os.remove, self._tmp_files)

https://codereview.chromium.org/2917473002/diff/60001/tools/web_dev_style/js_...
tools/web_dev_style/js_checker_eslint_test.py:29: _HERE_PATH =
os.path.dirname(__file__)
can we combine both __file__ references in a global variable (here and for the
sys.path.append)

https://codereview.chromium.org/2917473002/diff/60001/tools/web_dev_style/js_...
tools/web_dev_style/js_checker_eslint_test.py:40: resultsJson =
checker.RunEsLintChecks([tmp.name], format='json')
results_json

https://engdoc.corp.google.com/eng/doc/devguide/py/styleguide.shtml?cl=head#n...

[Dan Beam, 2017-05-30 23:12:27]

https://codereview.chromium.org/2917473002/diff/60001/tools/web_dev_style/js_...
File tools/web_dev_style/js_checker_eslint_test.py (right):

https://codereview.chromium.org/2917473002/diff/60001/tools/web_dev_style/js_...
tools/web_dev_style/js_checker_eslint_test.py:31: self._tmp_files.append(f.name)
why do we need an array still?

[dpapad, 2017-05-31 01:30:06]

Will add additional OWNERs once this LG.

https://codereview.chromium.org/2917473002/diff/1/tools/web_dev_style/js_chec...
File tools/web_dev_style/js_checker.py (right):

https://codereview.chromium.org/2917473002/diff/1/tools/web_dev_style/js_chec...
tools/web_dev_style/js_checker.py:127: results +=
self.RunEsLintChecks(affected_js_files_paths)
On 2017/05/30 at 23:11:08, Dan Beam wrote:
> On 2017/05/30 22:11:30, dpapad wrote:
> > I had to change the signature of RunEsLintChecks, to be able to directly
call it
> > from tests. The problem is that even though I can setup a mock
> > input_api.AffectedFiles to return a MockFile instance, the MockFile instance
is
> > useless to ESLint, it needs an actual file.
> 
> wait, why doesn't this work?
>
https://cs.chromium.org/chromium/src/PRESUBMIT_test_mocks.py?sq=package:chrom...

Updated the code to use a MockFile().

https://codereview.chromium.org/2917473002/diff/60001/tools/web_dev_style/js_...
File tools/web_dev_style/js_checker_eslint_test.py (right):

https://codereview.chromium.org/2917473002/diff/60001/tools/web_dev_style/js_...
tools/web_dev_style/js_checker_eslint_test.py:13:
os.path.join(os.path.dirname(os.path.abspath(__file__)), '..', '..'))
On 2017/05/30 at 23:11:09, Dan Beam wrote:
> indent is weird (should only be 4 \s for continuation)

Done.

https://codereview.chromium.org/2917473002/diff/60001/tools/web_dev_style/js_...
tools/web_dev_style/js_checker_eslint_test.py:15: from PRESUBMIT_test_mocks
import (MockInputApi, MockOutputApi)
On 2017/05/30 at 23:11:09, Dan Beam wrote:
> nit: do you need the parens around the module names you're importing?

Done.

https://codereview.chromium.org/2917473002/diff/60001/tools/web_dev_style/js_...
tools/web_dev_style/js_checker_eslint_test.py:21: 
On 2017/05/30 at 23:11:09, Dan Beam wrote:
> only \n (not \n\n) between these.
> 
> \n\n is only for top-level

Done.

https://codereview.chromium.org/2917473002/diff/60001/tools/web_dev_style/js_...
tools/web_dev_style/js_checker_eslint_test.py:24: os.remove(tmp_file)
On 2017/05/30 at 23:11:09, Dan Beam wrote:
> arguable nit: map(os.remove, self._tmp_files)

Not needed anymore.

https://codereview.chromium.org/2917473002/diff/60001/tools/web_dev_style/js_...
tools/web_dev_style/js_checker_eslint_test.py:29: _HERE_PATH =
os.path.dirname(__file__)
On 2017/05/30 at 23:11:09, Dan Beam wrote:
> can we combine both __file__ references in a global variable (here and for the
sys.path.append)

Done.

https://codereview.chromium.org/2917473002/diff/60001/tools/web_dev_style/js_...
tools/web_dev_style/js_checker_eslint_test.py:31: self._tmp_files.append(f.name)
On 2017/05/30 at 23:12:27, Dan Beam wrote:
> why do we need an array still?

Removed the array.

https://codereview.chromium.org/2917473002/diff/60001/tools/web_dev_style/js_...
tools/web_dev_style/js_checker_eslint_test.py:40: resultsJson =
checker.RunEsLintChecks([tmp.name], format='json')
On 2017/05/30 at 23:11:09, Dan Beam wrote:
> results_json
> 
>
https://engdoc.corp.google.com/eng/doc/devguide/py/styleguide.shtml?cl=head#n...

Done.

[Dan Beam, 2017-05-31 01:40:31]

lgtm w/nits

https://codereview.chromium.org/2917473002/diff/80001/PRESUBMIT_test_mocks.py
File PRESUBMIT_test_mocks.py (right):

https://codereview.chromium.org/2917473002/diff/80001/PRESUBMIT_test_mocks.py...
PRESUBMIT_test_mocks.py:30: self.presubmitLocalPath = os.path.dirname(__file__)
presubmit_local_path

https://codereview.chromium.org/2917473002/diff/80001/tools/web_dev_style/js_...
File tools/web_dev_style/js_checker_eslint_test.py (right):

https://codereview.chromium.org/2917473002/diff/80001/tools/web_dev_style/js_...
tools/web_dev_style/js_checker_eslint_test.py:16:
os.path.join(os.path.dirname(_HERE_PATH), '..', '..'))
why do we need the dirname() around _HERE_PATH again?

https://codereview.chromium.org/2917473002/diff/80001/tools/web_dev_style/js_...
tools/web_dev_style/js_checker_eslint_test.py:27: suffix='.js', dir=_HERE_PATH,
delete=False) as f:
arguable nit for prettiness:

tmp_args = {suffix='.js', dir=_HERE_PATH, delete=False}
with tempfile.NamedTemporaryFile(**tmp_args) as f:

https://codereview.chromium.org/2917473002/diff/80001/tools/web_dev_style/js_...
tools/web_dev_style/js_checker_eslint_test.py:29: print self._tmp_file
remove print

[dpapad, 2017-05-31 02:49:57]

dpapad@chromium.org changed reviewers:
+ phajdan.jr@chromium.org

[dpapad, 2017-05-31 02:49:58]

@phajdan.jr: Please review PRESUBMIT_test_mocks.py change.

https://codereview.chromium.org/2917473002/diff/80001/tools/web_dev_style/js_...
File tools/web_dev_style/js_checker_eslint_test.py (right):

https://codereview.chromium.org/2917473002/diff/80001/tools/web_dev_style/js_...
tools/web_dev_style/js_checker_eslint_test.py:16:
os.path.join(os.path.dirname(_HERE_PATH), '..', '..'))
On 2017/05/31 at 01:40:31, Dan Beam wrote:
> why do we need the dirname() around _HERE_PATH again?

Removed.

https://codereview.chromium.org/2917473002/diff/80001/tools/web_dev_style/js_...
tools/web_dev_style/js_checker_eslint_test.py:27: suffix='.js', dir=_HERE_PATH,
delete=False) as f:
On 2017/05/31 at 01:40:31, Dan Beam wrote:
> arguable nit for prettiness:
> 
> tmp_args = {suffix='.js', dir=_HERE_PATH, delete=False}
> with tempfile.NamedTemporaryFile(**tmp_args) as f:

Done.

https://codereview.chromium.org/2917473002/diff/80001/tools/web_dev_style/js_...
tools/web_dev_style/js_checker_eslint_test.py:29: print self._tmp_file
On 2017/05/31 at 01:40:31, Dan Beam wrote:
> remove print

Done.

[Paweł Hajdan Jr., 2017-05-31 15:23:22]

LGTM

[dpapad, 2017-05-31 17:17:51]

The CQ bit was checked by dpapad@chromium.org

[dpapad, 2017-05-31 17:17:52]

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/2917473002/#ps40002
(title: "Address nits.")

[dpapad, 2017-05-31 17:17:54]

The CQ bit was unchecked by dpapad@chromium.org

[dpapad, 2017-05-31 17:19:21]

The CQ bit was checked by dpapad@chromium.org

[dpapad, 2017-05-31 17:19:26]

The CQ bit was unchecked by dpapad@chromium.org

[dpapad, 2017-05-31 20:21:24]

The CQ bit was checked by dpapad@chromium.org

[commit-bot: I haz the power, 2017-05-31 20:22: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-05-31 20:51:37]

CQ is committing da patch.

Bot data: {"patchset_id": 40002, "attempt_start_ts": 1496262084260810,
"parent_rev": "c916dee9e65e1e0040b8f52e34753c953121e61f", "commit_rev":
"5c9c24ec6d1d8ddaae83a9118c5824b6c40f9854"}

[commit-bot: I haz the power, 2017-05-31 20:51:44]

Description was changed from

==========
js_checker.py: Restore smoke tests for linting violations.

Restoring test to ensure that document.getElementById() calls are flagged as
style violations, now that these checks are performed via ESLint. These tests
are not meant to be exhaustive, instead they just validate that our PRESUBMIT
integration with ESLint works.

BUG=720034
==========

to

==========
js_checker.py: Restore smoke tests for linting violations.

Restoring test to ensure that document.getElementById() calls are flagged as
style violations, now that these checks are performed via ESLint. These tests
are not meant to be exhaustive, instead they just validate that our PRESUBMIT
integration with ESLint works.

BUG=720034

Review-Url: https://codereview.chromium.org/2917473002
Cr-Commit-Position: refs/heads/master@{#476014}
Committed:
https://chromium.googlesource.com/chromium/src/+/5c9c24ec6d1d8ddaae83a9118c58...
==========

[commit-bot: I haz the power, 2017-05-31 20:51:45]

Committed patchset #6 (id:40002) as
https://chromium.googlesource.com/chromium/src/+/5c9c24ec6d1d8ddaae83a9118c58...