Repository: jshint/jshint
Pull request #3335 (2018-10-29 09:27:06)

[[FIX]] Use relative paths with `--filename` when recieving from stdin
When linting stdin with the `--filename` option set, config overrides
that were relative to the current directory had no effect. This fixes that
issue, but existing workarounds will still work.

Fixes 2536 (Extra comment at the bottom)

This has been live-tested to fixup the issue identified in the test repo at https://github.com/bostrom/jshint-filename-test.

coveralls: 
[![Coverage Status](https://coveralls.io/builds/19775449/badge)](https://coveralls.io/builds/19775449)

Coverage remained the same at 98.755% when pulling **9b9cfe08792017c894fedc29bc8da822d5173968 on leamingrad:filename_stdin_overrides_fix** into **3a8ef8b612487f390520374cd175f8af1ad28e2a on jshint:master**.

leamingrad: At the high level, it seems just passing in the filename parameter will bring the behaviour in line with the non-stdin case.

Interestingly, I think that maybe there is a bug in minimatch here. In particular, when I was investigating I modified `testOverrides` to pass the same data in as a file and over stdin and interestingly the bug does not appear if the file is directly in the working directory. As a concrete example, with the config:

    {
      "asi": true,
      "overrides": {
        "bar.js": {
          "asi": false
        }
      }
    }

then violations of `asi` are correctly ignored in `bar.js` when it is passed in as a file (the testcase in `testOverrides`) and when it is passed in over stdin.

However, if this config is changed to:

    {
      "asi": true,
      "overrides": {
        "src/bar.js": {
          "asi": false
        }
      }
    }

then violations of `asi` are correctly ignored in `src/bar.js` when passed as a file but not when passed over stdin.

Adding some logging around line 476 (where we decide which overrides to apply) gives the following:

* Does `bar.js` match `bar.js`: Yes
* Does `bar.js` match `/path/to/working/dir/bar.js`: Yes
* Does `src/bar.js` match `src/bar.js`: Yes
* Does `bar.js` match `/path/to/working/dir/src/bar.js`: No

I am not sure if this is a bug in the matching or not, but either way we can work around this by not resolving the input to `--filename` when we pass it in to `lint`.
jugglinmike: That behavior is definitely confusing, and I share your reluctance to replicate it in this new "ignore" matching feature. That said, I also don't like the idea of using two different matching heuristics for such closely-related functionality.

The CLI documentation has this to say about `--filename`:

>       --filename STRING  Pass in a filename when using STDIN to emulate config 
>                          lookup for that file name

I think that definitively qualifies the "override" matching behavior you've identified as a bug. Do you agree? What do you say we fix that so we can have a single heuristic that makes intuitive sense?
leamingrad: I'm happy to fix, but I'm slightly confused about the desired behavior here. Am I right in thinking that you want the stdin overrides to work for absolute and relative paths?

As a concrete example, say the directory structure is:

    myfolder/
        config.json
        foo/
            bar.js

and `config.json` is:

    {
      "asi": true,
      "overrides": {
        "foo/bar.js": {
          "asi": false
        }
      }
    }

Then which of the following commands are expected to produce no errors when run with the working directory of `mydirectory` (assuming `bar.js` would trigger the `asi` error):

1) `jshint foo/`
2) `jshint foo/bar.js`
3) `jshint /path/to/mydirectory/foo/bar.js`
4) `cat foo/bar.js | jshint --filename foo/bar.js -`
5) `cat foo/bar.js | jshint --filename /path/to/mydirectory/foo/bar.js -`

Currently 1 and 2 have no errors and 3, 4 and 5 have errors. I think that 4 should definitely not have errors (as it is equivalent to 2).

I am not sure about 3 and 5 - I think they should behave the same way, and I can see that being useful for integrations (e.g. vim plugins that work with absolute paths etc).

Let me know what you want me to do about 3 and 5 then I'll add test coverage for all the relevant cases.
leamingrad: @jugglinmike I've got time to pick this up again after a busy few months - is this fix still needed, and if so do you have any feedback on my clarification question above?
jugglinmike: Glad to hear it! I'll need a few days before I can give this attention it deserves, but know that I've seen your question, and I'm excited to help get this patch landed.
jugglinmike: Let's leave case 3 and 5 as they are and focus on relative paths for this patch. I can see what you're suggesting about editor plugins, but I'm reluctant to make changes in the absence of a real-world motivation. We can revisit if/when someone needs that functionality.

Does that sound good to you?
leamingrad: @jugglinmike: Again, sorry for the delay with this, but I have reworked the MR to just cover ensuring that relative paths work when using `--filename` for all the variants that this can take. This should now be ready for re-review.
leamingrad: I'm not sure why this has broken the CI - my guess is that something has gone in since I opened this MR that my changes are incompatible with - any help would be appreciated.
jugglinmike: Click through to the report produced by TravisCI and you'll see that the tests are failing in Node.js version 0.10. That's a pretty ancient release, but compatibility is important to us, so we make sure things work in that environment.

Unfortunately in this case, the failure is unrelated to JSHint's behavior. The testing infrastructure is not recognizing the second command-line invocation, but only in Node.js 0.10. I traced this as far as the `mock-stdin` module, but I doubt anyone (not even our own @caitp, who happens to maintain that module) will be interested in fixing bugs for a long-deprecated release of Node.js. What we need is a workaround.

I recommend a change that will also improves test hygiene generally: create a separate test for each behavior. If you split tests like that in two, not only will they pass in Node.js version 0.10, they will also be more helpful when/if they fail (since the two parts will be able to fail independently).

If you'd like to avoid duplicating your setup logic, you can nest related tests and use the framework's `setUp` function. Here's a commit that shows how to do this for one of your tests:

https://github.com/leamingrad/jshint/compare/filename_stdin_overrides_fix...jugglinmike:leamingrad-demo?w=1

Do you think you could use that as a guide to alter the other tests?
leamingrad: Thanks for the help - I have split all of the tests and everything is now passing.
leamingrad: @jugglinmike - I think this is probably in a state to merge. I don't really have time to work on this these days, so can I get a steer whether it is worth merging this as-is or just leaving?
jugglinmike: Sure. The remaining change was only indirectly related to your patch, so I'll follow up with a pull request of my own.

Thanks for all your hard work on this!

Patch:
diff --git a/src/cli.js b/src/cli.js
index 7933ca67..4a749034 100644
--- a/src/cli.js
+++ b/src/cli.js
@@ -611,15 +611,24 @@ var exports = {
     }
 
     var filename;
+    var lintStdinFile;
 
     // There is an if(filename) check in the lint() function called below.
     // passing a filename of undefined is the same as calling the function
     // without a filename.  If there is no opts.filename, filename remains
     // undefined and lint() is effectively called with 4 parameters.
     if (opts.filename) {
-      filename = path.resolve(opts.filename);
+      filename = opts.filename;
+      var ignores = !opts.ignores ? loadIgnores({ cwd: opts.cwd }) :
+                                  opts.ignores.map(function(target) {
+                                    return path.resolve(target);
+                                  });
+      lintStdinFile = (opts.useStdin && !isIgnored(filename, ignores));
+    } else {
+      lintStdinFile = opts.useStdin;
     }
-    if (opts.useStdin && opts.ignores.indexOf(filename) === -1) {
+
+    if (lintStdinFile) {
       cli.withStdin(function(code) {
         var config = opts.config;
 


Test patch:
diff --git a/tests/cli.js b/tests/cli.js
index 11624474..7930c417 100644
--- a/tests/cli.js
+++ b/tests/cli.js
@@ -264,6 +264,7 @@ exports.group = {
     test.done();
   },
 
+  // Overrides should work for files in the current directory
   testOverrides: function (test) {
     var dir = __dirname + "/../examples/";
     var rep = require("../examples/reporter.js");
@@ -302,13 +303,14 @@ exports.group = {
     cli.interpret([
       "node", "jshint", "bar.js", "--config", "config.json", "--reporter", "reporter.js"
     ]);
-    test.ok(rep.reporter.args[1][0].length > 0, "Error was expected but not thrown");
+    test.ok(rep.reporter.args[1][0].length === 1, "Error was expected but not thrown");
     test.equal(rep.reporter.args[1][0][0].error.code, "W033");
 
     test.done();
   },
 
-  testOverridesMatchesRelativePaths: function (test) {
+  // Overrides should work for implicit relative paths (without a leading ./)
+  testOverridesMatchesImplicitRelativePaths: function (test) {
     var dir = __dirname + "/../examples/";
     var rep = require("../examples/reporter.js");
     var config = {
@@ -323,21 +325,76 @@ exports.group = {
     this.sinon.stub(process, "cwd").returns(dir);
     this.sinon.stub(rep, "reporter");
     this.sinon.stub(shjs, "cat")
+      .withArgs(sinon.match(/foo\.js$/)).returns("a()")
       .withArgs(sinon.match(/bar\.js$/)).returns("a()")
       .withArgs(sinon.match(/config\.json$/))
         .returns(JSON.stringify(config));
 
     this.sinon.stub(shjs, "test")
+      .withArgs("-e", sinon.match(/foo\.js$/)).returns(true)
       .withArgs("-e", sinon.match(/bar\.js$/)).returns(true)
       .withArgs("-e", sinon.match(/config\.json$/)).returns(true);
 
     cli.exit.withArgs(0).returns(true)
       .withArgs(1).throws("ProcessExit");
 
+    // Test successful file
+    cli.interpret([
+      "node", "jshint", "src/foo.js", "--config", "config.json", "--reporter", "reporter.js"
+    ]);
+    test.ok(rep.reporter.args[0][0].length === 0);
+
+    // Test overriden, failed file
+    cli.interpret([
+      "node", "jshint", "src/bar.js", "--config", "config.json", "--reporter", "reporter.js"
+    ]);
+    test.ok(rep.reporter.args[1][0].length === 1, "Error was expected but not thrown");
+    test.equal(rep.reporter.args[1][0][0].error.code, "W033");
+
+    test.done();
+  },
+
+  // Overrides should work for explicit relative paths (with a leading ./)
+  testOverridesMatchesExplicitRelativePaths: function (test) {
+    var dir = __dirname + "/../examples/";
+    var rep = require("../examples/reporter.js");
+    var config = {
+      "asi": true,
+      "overrides": {
+        "src/bar.js": {
+          "asi": false
+        }
+      }
+    };
+
+    this.sinon.stub(process, "cwd").returns(dir);
+    this.sinon.stub(rep, "reporter");
+    this.sinon.stub(shjs, "cat")
+      .withArgs(sinon.match(/foo\.js$/)).returns("a()")
+      .withArgs(sinon.match(/bar\.js$/)).returns("a()")
+      .withArgs(sinon.match(/config\.json$/))
+        .returns(JSON.stringify(config));
+
+    this.sinon.stub(shjs, "test")
+      .withArgs("-e", sinon.match(/foo\.js$/)).returns(true)
+      .withArgs("-e", sinon.match(/bar\.js$/)).returns(true)
+      .withArgs("-e", sinon.match(/config\.json$/)).returns(true);
+
+    cli.exit.withArgs(0).returns(true)
+      .withArgs(1).throws("ProcessExit");
+
+    // Test successful file
+    cli.interpret([
+      "node", "jshint", "./src/foo.js", "--config", "config.json", "--reporter", "reporter.js"
+    ]);
+    test.ok(rep.reporter.args[0][0].length === 0);
+
+    // Test overriden, failed file
     cli.interpret([
       "node", "jshint", "./src/bar.js", "--config", "config.json", "--reporter", "reporter.js"
     ]);
-    test.ok(rep.reporter.args[0][0].length === 1);
+    test.ok(rep.reporter.args[1][0].length === 1, "Error was expected but not thrown");
+    test.equal(rep.reporter.args[1][0][0].error.code, "W033");
 
     test.done();
   },
@@ -1416,5 +1473,237 @@ exports.useStdin = {
     test.equal(cli.exit.args[1][0], 0, "The input is not linted because the specified file name is ignored.");
 
     test.done();
+  },
+
+  // Overrides should work when the passed stdin filename is in the current directoy
+  testOverridesFromStdin: {
+    setUp: function(done) {
+      var dir = __dirname + "/../examples/";
+      this.rep = require("../examples/reporter.js");
+      var config = {
+        "asi": true,
+        "overrides": {
+          "bar.js": {
+            "asi": false
+          }
+        }
+      };
+
+      this.sinon.stub(process, "cwd").returns(dir);
+      this.sinon.stub(this.rep, "reporter");
+      this.sinon.stub(shjs, "cat")
+        .withArgs(sinon.match(/config\.json$/))
+          .returns(JSON.stringify(config));
+
+      this.sinon.stub(shjs, "test")
+        .withArgs("-e", sinon.match(/config\.json$/)).returns(true);
+
+      cli.exit.withArgs(0).returns(true)
+        .withArgs(1).throws("ProcessExit");
+
+      done();
+    },
+
+    // Test successful file
+    success: function(test) {
+      cli.interpret([
+        "node", "jshint", "--filename", "foo.js", "--config", "config.json", "--reporter", "reporter.js", "-"
+      ]);
+      this.stdin.send("a()");
+      this.stdin.end();
+      test.ok(this.rep.reporter.args[0][0].length === 0);
+      test.done();
+    },
+
+    // Test overriden, failed file
+    failure: function(test) {
+      cli.interpret([
+        "node", "jshint", "--filename", "bar.js", "--config", "config.json", "--reporter", "reporter.js", "-"
+      ]);
+      this.stdin.send("a()");
+      this.stdin.end();
+      test.ok(this.rep.reporter.args[0][0].length === 1, "Error was expected but not thrown");
+      test.equal(this.rep.reporter.args[0][0][0].error.code, "W033");
+
+      test.done();
+    }
+  },
+
+  // Override behaviour for implicit relative paths (without a leading ./) should be the same
+  // for stdin as for file
+  testOverridesMatchesImplicitRelativePathsFromStdin: {
+    setUp: function (done) {
+      var dir = __dirname + "/../examples/";
+      this.rep = require("../examples/reporter.js");
+      var config = {
+        "asi": true,
+        "overrides": {
+          "src/bar.js": {
+            "asi": false
+          }
+        }
+      };
+
+      this.sinon.stub(process, "cwd").returns(dir);
+      this.sinon.stub(this.rep, "reporter");
+      this.sinon.stub(shjs, "cat")
+        .withArgs(sinon.match(/config\.json$/))
+          .returns(JSON.stringify(config));
+
+      this.sinon.stub(shjs, "test")
+        .withArgs("-e", sinon.match(/config\.json$/)).returns(true);
+
+      cli.exit.withArgs(0).returns(true)
+        .withArgs(1).throws("ProcessExit");
+
+      done();
+    },
+
+    // Test successful file
+    success: function(test) {
+      cli.interpret([
+        "node", "jshint", "--filename", "src/foo.js", "--config", "config.json", "--reporter", "reporter.js", "-"
+      ]);
+      this.stdin.send("a()");
+      this.stdin.end();
+      test.ok(this.rep.reporter.args[0][0].length === 0);
+      test.done();
+    },
+
+    // Test overriden, failed file
+    failure: function(test) {
+      cli.interpret([
+        "node", "jshint", "--filename", "src/bar.js", "--config", "config.json", "--reporter", "reporter.js", "-"
+      ]);
+      this.stdin.send("a()");
+      this.stdin.end();
+      test.ok(this.rep.reporter.args[0][0].length === 1, "Error was expected but not thrown");
+      test.equal(this.rep.reporter.args[0][0][0].error.code, "W033");
+
+      test.done();
+    }
+  },
+
+  // Override behaviour for explicit relative paths (with a leading ./) should be the same
+  // for stdin as for file
+  testOverridesMatchesExplicitRelativePathsFromStdin: {
+    setUp: function(done) {
+      var dir = __dirname + "/../examples/";
+      this.rep = require("../examples/reporter.js");
+      var config = {
+        "asi": true,
+        "overrides": {
+          "src/bar.js": {
+            "asi": false
+          }
+        }
+      };
+
+      this.sinon.stub(process, "cwd").returns(dir);
+      this.sinon.stub(this.rep, "reporter");
+      this.sinon.stub(shjs, "cat")
+        .withArgs(sinon.match(/config\.json$/))
+          .returns(JSON.stringify(config));
+
+      this.sinon.stub(shjs, "test")
+        .withArgs("-e", sinon.match(/config\.json$/)).returns(true);
+
+      cli.exit.withArgs(0).returns(true)
+        .withArgs(1).throws("ProcessExit");
+
+      done();
+    },
+
+    // Test successful file
+    success: function(test) {
+      cli.interpret([
+        "node", "jshint", "--filename", "./src/foo.js", "--config", "config.json", "--reporter", "reporter.js", "-"
+      ]);
+      this.stdin.send("a()");
+      this.stdin.end();
+      test.ok(this.rep.reporter.args[0][0].length === 0);
+      test.done();
+    },
+
+    // Test overriden, failed file
+    failure: function(test) {
+      cli.interpret([
+        "node", "jshint", "--filename", "./src/bar.js", "--config", "config.json", "--reporter", "reporter.js", "-"
+      ]);
+      this.stdin.send("a()");
+      this.stdin.end();
+      test.ok(this.rep.reporter.args[0][0].length === 1, "Error was expected but not thrown");
+      test.equal(this.rep.reporter.args[0][0][0].error.code, "W033");
+
+      test.done();
+    }
+  },
+
+  // We should only lint stdin when the special `-` or `/dev/stdin` values are passed, even
+  // if the `--filename` parameter is set
+  testStdinInputCorrectlyIgnored: {
+    setUp: function(done) {
+      var dir = __dirname + "/../examples/";
+      this.rep = require("../examples/reporter.js");
+      var config = {
+        "asi": false,
+        "overrides": {
+          "src/foo.js": {
+            "asi": true
+          }
+        }
+      };
+
+      this.sinon.stub(process, "cwd").returns(dir);
+      this.sinon.stub(this.rep, "reporter");
+      this.sinon.stub(shjs, "cat")
+        .withArgs(sinon.match(/foo\.js$/)).returns("a()")
+        .withArgs(sinon.match(/config\.json$/))
+          .returns(JSON.stringify(config));
+
+      this.sinon.stub(shjs, "test")
+        .withArgs("-e", sinon.match(/config\.json$/)).returns(true)
+        .withArgs("-e", sinon.match(/foo\.js$/)).returns(true);
+
+      cli.exit.withArgs(0).returns(true)
+        .withArgs(1).throws("ProcessExit");
+
+      done();
+    },
+
+    // Check that the stdin that is used in this test group actually triggers an error
+    stdinUsed: function(test) {
+      cli.interpret([
+        "node", "jshint", "--config", "config.json", "--reporter", "reporter.js", "-"
+      ]);
+      this.stdin.send("a()");
+      this.stdin.end();
+      test.ok(this.rep.reporter.args[0][0].length === 1, "Error was expected but not thrown");
+      test.equal(this.rep.reporter.args[0][0][0].error.code, "W033");
+
+      test.done();
+    },
+
+    // If the filename is set to something other than stdin, then sending stdin should be ignored
+    withoutFilename: function(test) {
+      cli.interpret([
+        "node", "jshint", "--config", "config.json", "--reporter", "reporter.js", "./src/foo.js"
+      ]);
+      this.stdin.send("a()");
+      this.stdin.end();
+      test.ok(this.rep.reporter.args[0][0].length === 0);
+      test.done();
+    },
+
+    // Passing the filename parameter should make no difference
+    withFilename: function(test) {
+      cli.interpret([
+        "node", "jshint", "--filename", "./src/bar.js", "--config", "config.json", "--reporter", "reporter.js", "./src/foo.js"
+      ]);
+      this.stdin.send("a()");
+      this.stdin.end();
+      test.ok(this.rep.reporter.args[0][0].length === 0);
+      test.done();
+    }
   }
 };