Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

test: Fix make jslint to include test scripts too. #25280

Closed
saquibkhan opened this issue May 12, 2015 · 10 comments
Closed

test: Fix make jslint to include test scripts too. #25280

saquibkhan opened this issue May 12, 2015 · 10 comments

Comments

@saquibkhan
Copy link

Lint doesn't run on test scripts and errors are caught at later stage during code review.

We should fix make jshint to include test scripts too.

@saquibkhan saquibkhan changed the title test: Fixing make jslint to include test scripts too. test: Fix make jslint to include test scripts too. May 12, 2015
@saquibkhan saquibkhan changed the title test: Fix make jslint to include test scripts too. test: Fix make jshint to include test scripts too. May 12, 2015
@saquibkhan
Copy link
Author

Can you give your feedback? /cc @joyent/node-coreteam @misterdjules

@jasnell
Copy link
Member

jasnell commented May 12, 2015

+1 this would definitely be helpful. PR? :-D

@saquibkhan
Copy link
Author

@jasnell I can work on this if coreteam thinks its useful.
Should the PR go in master?

@jasnell
Copy link
Member

jasnell commented May 12, 2015

Yes, for something like this, master would be the target.

/cc @joyent/node-coreteam

@saquibkhan
Copy link
Author

Please find lint log after we run it on test
Found 2090 errors, including 814 new errors, in 302 files (742 files OK).

@misterdjules misterdjules changed the title test: Fix make jshint to include test scripts too. test: Fix make jslint to include test scripts too. May 12, 2015
@misterdjules
Copy link

@saquibkhan Just to make sure there's no confusion, jshint is not used by node currently to lint JavaScript source, instead it uses Closure.

@saquibkhan
Copy link
Author

@misterdjules Can you tell me how to fix Multi-line strings are not allowed ?

saquibkhan pushed a commit to saquibkhan/node that referenced this issue May 12, 2015
…int errors

Added closure linter for test folder. Fixed 2k+ issues in 900 files.

Fixes nodejs#25280
@saquibkhan
Copy link
Author

@misterdjules @jasnell Please find the PR #25288 for this.
Fixed 2090 errors in 301 files. All lint issues fixed
All Test Passed

Logs below:

make lint
PYTHONPATH=tools/closure_linter/ /usr/bin/python tools/closure_linter/closure_linter/gjslint.py --unix_mode --strict --nojsdoc -r lib/ -r src/ -r test/ --exclude_files lib/punycode.js,test/fixtures/throws_error4.js,test/fixtures/uncaught-exceptions/parse-error-mod.js,test/message/throw_in_line_with_tabs.js
Skipping 4 file(s).
1041 files checked, no errors found.
Done processing src/async-wrap.cc
Done processing src/cares_wrap.cc
Done processing src/fs_event_wrap.cc
Done processing src/handle_wrap.cc
Done processing src/node.cc
Done processing src/node_buffer.cc
Done processing src/node_constants.cc
Done processing src/node_contextify.cc
Done processing src/node_counters.cc
Done processing src/node_crypto.cc
Done processing src/node_crypto_bio.cc
Done processing src/node_crypto_clienthello.cc
Done processing src/node_dtrace.cc
Done processing src/node_file.cc
Done processing src/node_http_parser.cc
Done processing src/node_i18n.cc
Done processing src/node_javascript.cc
Done processing src/node_main.cc
Done processing src/node_os.cc
Done processing src/node_stat_watcher.cc
Done processing src/node_v8.cc
Done processing src/node_watchdog.cc
Done processing src/node_win32_etw_provider.cc
Done processing src/node_zlib.cc
Done processing src/pipe_wrap.cc
Done processing src/process_wrap.cc
Done processing src/signal_wrap.cc
Done processing src/smalloc.cc
Done processing src/spawn_sync.cc
Done processing src/stream_wrap.cc
Done processing src/string_bytes.cc
Done processing src/tcp_wrap.cc
Done processing src/timer_wrap.cc
Done processing src/tls_wrap.cc
Done processing src/tty_wrap.cc
Done processing src/udp_wrap.cc
Done processing src/util.cc
Done processing src/uv.cc
Done processing src/async-wrap-inl.h
Done processing src/async-wrap.h
Done processing src/base-object-inl.h
Done processing src/base-object.h
Done processing src/env-inl.h
Done processing src/env.h
Done processing src/handle_wrap.h
Done processing src/node.h
Done processing src/node_buffer.h
Done processing src/node_constants.h
Done processing src/node_counters.h
Done processing src/node_crypto.h
Done processing src/node_crypto_bio.h
Done processing src/node_crypto_clienthello-inl.h
Done processing src/node_crypto_clienthello.h
Done processing src/node_crypto_groups.h
Done processing src/node_dtrace.h
Done processing src/node_file.h
Done processing src/node_http_parser.h
Done processing src/node_i18n.h
Done processing src/node_internals.h
Done processing src/node_javascript.h
Done processing src/node_object_wrap.h
Done processing src/node_stat_watcher.h
Done processing src/node_version.h
Done processing src/node_watchdog.h
Done processing src/node_win32_etw_provider-inl.h
Done processing src/node_win32_etw_provider.h
Done processing src/node_win32_perfctr_provider.h
Done processing src/node_wrap.h
Done processing src/pipe_wrap.h
Done processing src/req_wrap.h
Done processing src/smalloc.h
Done processing src/spawn_sync.h
Done processing src/stream_wrap.h
Done processing src/string_bytes.h
Done processing src/tcp_wrap.h
Done processing src/tls_wrap.h
Done processing src/tty_wrap.h
Done processing src/udp_wrap.h
Done processing src/util-inl.h
Done processing src/util.h
Done processing tools/icu/iculslocs.cc
Done processing tools/icu/no-op.cc
Done processing deps/debugger-agent/include/debugger-agent.h
Done processing deps/debugger-agent/src/agent.cc
Done processing deps/debugger-agent/src/agent.h
Total errors found: 0

saquibkhan pushed a commit to saquibkhan/node that referenced this issue May 12, 2015
…int errors

Added closure linter for test folder. Fixed 2k+ issues in 900 files.

Fixes nodejs#25280
saquibkhan added a commit to saquibkhan/node that referenced this issue May 13, 2015
Added closure linter for test folder. Fixed 2k+ issues in 900 files.
After this fix there is no lint error in test scripts.

Fixes nodejs#25280
saquibkhan added a commit to saquibkhan/node that referenced this issue May 13, 2015
Added closure linter for test folder. Fixed 2k+ issues in 300 files.
After this fix there is no lint error in test scripts.

Fixes nodejs#25280
saquibkhan added a commit to saquibkhan/node that referenced this issue May 13, 2015
Added closure linter for test folder. Fixed 2k+ issues in 300 files.
After this fix there is no lint error in test scripts.

Fixes nodejs#25280
@silverwind
Copy link

FYI, we've switched to eslint on io.js so this can only land on the 0.12 branch. A bit of unfortunate that I haven't noticed this issue before, or else I'd have adviced doing it on the io.js tree.

nodejs/node#1721 is pending to include test linting for eslint.

@brendanashworth
Copy link

I doubt this will land on the 0.12 branch, so I'll close this. Eslint is now used in node 4, anyways.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants