Added linting for JavaScript tests.

[nikolas]

No description provided.

[timgraham]

Thanks for the patch. I think it would be great if we could add automatic linting to the tests to prevent future regressions in this area. Do you think you could apply the below patch and fix all the errors? @treyhunner -- sound good?

diff --git a/package.json b/package.json
index d926bdc..04da8db 100644
--- a/package.json
+++ b/package.json
@@ -2,7 +2,7 @@
   "name": "Django",
   "private": true,
   "scripts": {
-    "pretest": "eslint django/",
+    "pretest": "eslint django/ js_tests/admin",
     "test": "grunt test --verbose"
   },
   "devDependencies": {

[nikolas]

Alright, I just added a new commit adding that line to package.json, and validating js_tests/admin/ with eslint.

[nikolas]

I'm not sure why the jenkins build is showing new eslint errors. I used the latest version of eslint (version 1.2.1).

[timgraham]

You aren't able to reproduce the errors reported in http://djangoci.com/job/pull-requests-javascript/944/console?

[nikolas]

Correct, eslint js_tests/admin/ exits cleanly for me. I can reproduce the errors in the django/ dir when running eslint django.

[timgraham]

How about with npm test as documented here: https://docs.djangoproject.com/en/dev/internals/contributing/writing-code/javascript/#testing-from-the-command-line?

[nikolas]

I just ran npm test and got the same results. I see errors in the django directory, but not in js_tests. Tested this on both my local machine and a Linode VPS, in Ubuntu.

[timgraham]

Not sure... could you put the commands output in a pastebin?

[nikolas]

Sure, here's what it looks like: http://dpaste.com/133CK1M

[timgraham]

npm ls eslint gives eslint@0.22.1 for me. This matches "eslint": "^0.22.1" in package.json. Maybe the different version you have installed is the reason for the discrepancy.

[nikolas]

"eslint": "^0.22.1" means that django requires any version of eslint, 0.22.1 or later. Since there can be lots of these kinds of discrepancies between versions, I suggest we pin this to a specific version of eslint, like "eslint": "0.22.1".

[timgraham]

Maybe we should. I'm not an expert, but from what I read, "The ^ in the version is a shorthand for >=0.22.1 < 1.0.0. " I guess the rationale is that backwards compatibility should be provided through the next major release. We'll see if Trey chimes in (he did the original work). Did 1.2.1 get installed through npm install or through some other method? When I did the install on a fresh box I did get 0.22.1.

[nikolas]

Ah okay. It was my mistake then. I interpreted "^0.22.1" as "0.22.1 and up", and installed the latest version of eslint in my home directory from outside of the django directory.

I just installed eslint via npm install inside the django directory and got version 0.22.1. I'll fix up this branch so it's passing now.

[treyhunner]

I think it would be great if we could add automatic linting to the tests to prevent future regressions in this area

@timgraham I agree. I do think the check should be separate from the test check so it fails independently.

[treyhunner]

Since these are all test files, I think all these 'use strict' statements at the top of each function could be replaced with a single 'use strict' at the top of each file.

[nikolas]

When I try that I get these errors:

js_tests/admin/RelatedObjectLookups.test.js
   3:0   warning  Use the function form of "use strict"  strict
   3:0   error    Use the function form of "use strict"  global-strict

[timgraham]

'use_strict' in every test does seem like boilerplate. Any other alternatives?

[nikolas]

Yeah, I agree it's pretty unnecessary. We could add "strict": 0 to the .eslintrc as an alternative.

[timgraham]

I guess it could be nice to enforce strictness on the JavaScript code, but not the tests?

[nikolas]

In that case we can add /* eslint strict: 0 */ to the beginning of each test file. I just tried that and it works.

[timgraham]

That seems okay and the PR otherwise looks okay to me. Trey, any comments?

[treyhunner]

This works, but I would prefer to enforce strict mode on all code if possible.

I would either suppress the warnings about the global "use strict" in each file or wrap each test file in an immediately-invoked function expression.

[nikolas]

I've added a new commit to use the global form of 'use strict'; here.

[timgraham]

Looks good to me. Okay with you, Trey?

[treyhunner]

👍 looks good

An aside: ESLint 1.0 was released about a month ago. global-strict and some other properties will be removed in 1.0 as properties are merged together.

[nikolas]

That's good to know. Maybe I'll make a new PR for eslint 1.x once this is merged.

[timgraham]

Sounds great about updating to eslint 1.x.

Merged in 722bf23, thanks!