Repository: apiaryio/dredd (js)

Issue:
If additional files are given in --path and they don't exist, Dredd silently ignores them
Is this bug or feature? In case Dredd should ignore them, it should be documented and there should be at least warning logged. In case Dredd should fail, then it does not and it's a bug.

``` shell
$ ls
apiary.apib
$ dredd ./apiary.apib http://localhost:8000 --path=./non-existent.apib
...
$ echo $?
0
```


Pull request:
Throw error when additional path doesn't exist
#### :rocket: Why this change?

Dredd should treat non-existing files (or globs evaluating to no files) as user error. This PR just finalizes @jgjadaoag's amazing work in https://github.com/apiaryio/dredd/pull/909.

We want to have this done as soon as possible so there are no open Pull Requests and we could decaf the codebase - #705.

#### :memo: Related issues and Pull Requests

- Fixes #378
- Closes https://github.com/apiaryio/dredd/pull/909

#### :white_check_mark: What didn't I forget?

- [ ] ~~To write docs~~
- [x] To write tests
- [x] To put [Conventional Changelog](https://dredd.readthedocs.io/en/latest/contributing/#sem-rel) prefixes in front of all my commits and run `npm run lint`


Patch:
diff --git a/src/add-hooks.coffee b/src/add-hooks.coffee
index 05a5fe69..8b5d098e 100644
--- a/src/add-hooks.coffee
+++ b/src/add-hooks.coffee
@@ -1,7 +1,6 @@
 require 'coffee-script/register'
 path = require 'path'
 proxyquire = require('proxyquire').noCallThru()
-glob = require 'glob'
 fs = require 'fs'
 async = require 'async'
 clone = require 'clone'
@@ -9,16 +8,13 @@ clone = require 'clone'
 Hooks = require './hooks'
 logger = require './logger'
 sandboxHooksCode = require './sandbox-hooks-code'
+resolveHookfiles = require './resolve-hookfiles'
 mergeSandboxedHooks = require './merge-sandboxed-hooks'
 HooksWorkerClient = require './hooks-worker-client'
 
-# Ensure platform agnostic path.basename function
-basename = if process.platform is 'win32' then path.win32.basename else path.basename
 
+# Note: runner.configuration.options must be defined
 addHooks = (runner, transactions, callback) ->
-  # Note: runner.configuration.options must be defined
-
-  customConfigCwd = runner?.configuration?.custom?.cwd
 
   fixLegacyTransactionNames = (allHooks) ->
     pattern = /^\s>\s/g
@@ -97,35 +93,14 @@ addHooks = (runner, transactions, callback) ->
       # No data found, doing nothing
       return callback()
 
-  # Loading hookfiles from fs
   else
-    # Expand hookfiles - sort files alphabetically and resolve their paths
-    hookfiles = [].concat runner.configuration?.options?.hookfiles
-    files = hookfiles.reduce((result, unresolvedPath) ->
-      # glob.sync does not resolve paths, only glob patterns
-      unresolvedPaths = if glob.hasMagic(unresolvedPath) then glob.sync(unresolvedPath) else [unresolvedPath]
-
-      # Gradually append sorted and resolved paths
-      result.concat unresolvedPaths
-        # Create a filename / filepath map for easier sorting
-        # Example:
-        # [
-        #   { basename: 'filename1.coffee', path: './path/to/filename1.coffee' }
-        #   { basename: 'filename2.coffee', path: './path/to/filename2.coffee' }
-        # ]
-        .map((filepath) -> basename: basename(filepath), path: filepath)
-        # Sort 'em up
-        .sort((a, b) -> switch
-          when a.basename < b.basename then -1
-          when a.basename > b.basename then 1
-          else 0
-        )
-        # Resolve paths to absolute form. Take into account user defined current
-        # working directory, fallback to process.cwd() otherwise
-        .map((item) -> path.resolve(customConfigCwd or process.cwd(), item.path))
-    , [] # Start with empty result
-    )
-
+    # Loading hookfiles from fs
+    hookfiles = [].concat(runner.configuration?.options?.hookfiles)
+    cwd = runner?.configuration?.custom?.cwd
+    try
+      files = resolveHookfiles(hookfiles, cwd)
+    catch err
+      return callback(err)
     logger.info('Found Hookfiles:', files)
 
     # Clone the configuration object to hooks.configuration to make it
diff --git a/src/dredd.coffee b/src/dredd.coffee
index d2c192b7..3198337d 100644
--- a/src/dredd.coffee
+++ b/src/dredd.coffee
@@ -126,18 +126,17 @@ class Dredd
       glob globToExpand, (err, match) =>
         return globCallback(err) if err
         @configuration.files = @configuration.files.concat match
+        if match.length == 0
+          err = new Error("""\
+            API description document(s) not found on path: \
+            '#{globToExpand}'\
+          """)
+          return globCallback(err)
         globCallback()
 
     , (err) =>
       return callback(err, @stats) if err
 
-      if @configDataIsEmpty and @configuration.files.length == 0
-        err = new Error("""\
-          API description document (or documents) not found on path: \
-          '#{@configuration.options.path}'\
-        """)
-        return callback(err, @stats)
-
       # remove duplicate filenames
       @configuration.files = removeDuplicates @configuration.files
       callback(null, @stats)
diff --git a/src/resolve-hookfiles.coffee b/src/resolve-hookfiles.coffee
new file mode 100644
index 00000000..c68892e4
--- /dev/null
+++ b/src/resolve-hookfiles.coffee
@@ -0,0 +1,39 @@
+fs = require('fs')
+path = require('path')
+glob = require('glob')
+
+
+# Ensure platform agnostic path.basename function
+basename = if process.platform is 'win32' then path.win32.basename else path.basename
+
+
+# Expand hookfiles - sort files alphabetically and resolve their paths
+resolveHookfiles = (hookfiles, cwd = null) ->
+  return [] if not hookfiles or not hookfiles.length
+  cwd ?= process.cwd()
+
+  resolvedPathsArrays = hookfiles.map((hookfile) ->
+    # glob.sync does not resolve paths, only glob patterns
+    if glob.hasMagic(hookfile)
+      resolvedPaths = glob.sync(hookfile, {cwd}).map((p) -> path.resolve(cwd, p))
+    else
+      resolvedPath = path.resolve(cwd, hookfile)
+      resolvedPaths = if fs.existsSync(resolvedPath) then [resolvedPath] else []
+
+    unless resolvedPaths.length
+      throw new Error("Could not find any hook file(s) on path: '#{hookfile}'")
+
+    return resolvedPaths
+  )
+  resolvedPaths = Array.concat.apply([], resolvedPathsArrays)
+  resolvedPaths = resolvedPaths.sort((p1, p2) ->
+    [p1, p2] = [basename(p1), basename(p2)]
+    switch
+      when p1 < p2 then -1
+      when p1 > p2 then 1
+      else 0
+  )
+  return resolvedPaths
+
+
+module.exports = resolveHookfiles


Test patch:
diff --git a/test/fixtures/hooks-glob/bar/b.js b/test/fixtures/hooks-glob/bar/b.js
new file mode 100644
index 00000000..e69de29b
diff --git a/test/fixtures/hooks-glob/bar/p.js b/test/fixtures/hooks-glob/bar/p.js
new file mode 100644
index 00000000..e69de29b
diff --git a/test/fixtures/hooks-glob/bar/z.js b/test/fixtures/hooks-glob/bar/z.js
new file mode 100644
index 00000000..e69de29b
diff --git a/test/fixtures/hooks-glob/baz/c.js b/test/fixtures/hooks-glob/baz/c.js
new file mode 100644
index 00000000..e69de29b
diff --git a/test/fixtures/hooks-glob/baz/x.js b/test/fixtures/hooks-glob/baz/x.js
new file mode 100644
index 00000000..e69de29b
diff --git a/test/fixtures/hooks-glob/foo/a.js b/test/fixtures/hooks-glob/foo/a.js
new file mode 100644
index 00000000..e69de29b
diff --git a/test/fixtures/hooks-glob/foo/o.js b/test/fixtures/hooks-glob/foo/o.js
new file mode 100644
index 00000000..e69de29b
diff --git a/test/fixtures/hooks-glob/foo/y.js b/test/fixtures/hooks-glob/foo/y.js
new file mode 100644
index 00000000..e69de29b
diff --git a/test/fixtures/hooks.js b/test/fixtures/hooks.js
new file mode 100644
index 00000000..e69de29b
diff --git a/test/fixtures/non-js-hooks.rb b/test/fixtures/non-js-hooks.rb
new file mode 100644
index 00000000..e69de29b
diff --git a/test/integration/cli/api-description-cli-test.coffee b/test/integration/cli/api-description-cli-test.coffee
index 8a47d055..0e02b6ec 100644
--- a/test/integration/cli/api-description-cli-test.coffee
+++ b/test/integration/cli/api-description-cli-test.coffee
@@ -268,7 +268,9 @@ describe 'CLI - API Description Document', ->
           runtimeInfo = info
           done(err)
 
-      it 'should request /machines', ->
-        assert.deepEqual runtimeInfo.server.requestCounts, {'/machines': 1}
-      it 'should exit with status 0', ->
-        assert.equal runtimeInfo.dredd.exitStatus, 0
+      it 'should not request server', ->
+        assert.isFalse runtimeInfo.server.requested
+      it 'should exit with status 1', ->
+        assert.equal runtimeInfo.dredd.exitStatus, 1
+      it 'should print error message to stderr', ->
+        assert.include runtimeInfo.dredd.stderr, 'not found'
diff --git a/test/integration/cli/cli-test.coffee b/test/integration/cli/cli-test.coffee
index 83a399d9..c59cd168 100644
--- a/test/integration/cli/cli-test.coffee
+++ b/test/integration/cli/cli-test.coffee
@@ -198,7 +198,7 @@ describe 'CLI', ->
           app.get '/machines', (req, res) ->
             # path.posix|win32.normalize and path.join do not do the job in this case,
             # hence this ingenious hack
-            normalizedPath = path.normalize('foo/bar/hooks').replace(/\\/g, '\\\\')
+            normalizedPath = path.normalize('test/fixtures/hooks.js').replace(/\\/g, '\\\\')
             killAll("endless-ignore-term.+[^=]#{normalizedPath}", (err) ->
               done err if err
               res.json([{type: 'bulldozer', name: 'willy'}])
@@ -215,7 +215,7 @@ describe 'CLI', ->
             "--server=#{COFFEE_BIN} ./test/fixtures/scripts/endless-ignore-term.coffee"
             '--server-wait=0'
             "--language=#{COFFEE_BIN} ./test/fixtures/scripts/endless-ignore-term.coffee"
-            '--hookfiles=foo/bar/hooks'
+            '--hookfiles=test/fixtures/hooks.js'
           ]
           hookHandler.listen DEFAULT_HOOK_HANDLER_PORT, ->
             runDreddCommandWithServer(args, app, (err, info) ->
diff --git a/test/unit/add-hooks-test.coffee b/test/unit/add-hooks-test.coffee
index f3b9776a..3a8d399f 100644
--- a/test/unit/add-hooks-test.coffee
+++ b/test/unit/add-hooks-test.coffee
@@ -95,7 +95,7 @@ describe 'addHooks(runner, transactions, callback)', () ->
         configuration:
           options:
             language: 'ruby'
-            hookfiles: './some/ruby/file.rb'
+            hookfiles: './test/fixtures/non-js-hooks.rb'
 
       sinon.stub(hooksWorkerClientStub.prototype, 'start').callsFake (cb) -> cb()
 
@@ -125,93 +125,17 @@ describe 'addHooks(runner, transactions, callback)', () ->
         globStub.sync.restore()
         done()
 
-    it 'should return files alphabetically sorted', (done) ->
-      runner =
-        configuration:
-          options:
-            hookfiles: ['./test/**/*_hooks.*', '/baz/x.js', '/foo/y.js', '/bar/z.js', '/foo/a.js', '/bar/b.js', '/baz/c.js', '/foo/o.js', '/bar/p.js']
-
-      addHooks runner, transactions, (err) ->
-        return done err if err
-
-        # We need >10 files to prove that sorting is ok
-        expected = [
-          'a.js',
-          'b.js',
-          'c.js',
-          'multifile_hooks.coffee',
-          'o.js',
-          'p.js',
-          'test2_hooks.js',
-          'test_hooks.coffee',
-          'x.js',
-          'y.js',
-          'z.js',
-        ]
-
-        actual = runner.hooks.configuration.options.hookfiles
-
-        assert.notEqual actual.length, 0
-
-        actual.forEach (item, index) ->
-          assert.include item, expected[index]
-
-        done()
-
     it 'should return files with resolved paths', (done) ->
       addHooks runner, transactions, (err) ->
         return done err if err
 
-        expected = [
+        assert.deepEqual runner.hooks.configuration.options.hookfiles, [
           pathStub.resolve(process.cwd(), './test/fixtures/multifile/multifile_hooks.coffee'),
           pathStub.resolve(process.cwd(), './test/fixtures/test2_hooks.js'),
           pathStub.resolve(process.cwd(), './test/fixtures/test_hooks.coffee')
         ]
-
-        actual = runner.hooks.configuration.options.hookfiles
-
-        assert.deepEqual actual, expected
-
-        done()
-
-    it 'should return resolved path for non existing file', (done) ->
-      runner =
-        configuration:
-          options:
-            hookfiles: 'foo/bar/hooks'
-
-      addHooks runner, transactions, (err) ->
-        return done err if err
-
-        actual = runner.hooks.configuration.options.hookfiles
-        expected = [pathStub.resolve(process.cwd(), 'foo/bar/hooks')]
-
-        assert.deepEqual actual, expected
-
-        done()
-
-    it 'should handle mixed filepaths and globs', (done) ->
-      runner =
-        configuration:
-          options:
-            hookfiles: ['foo/bar/hooks', './test/**/*_hooks.*']
-
-      addHooks runner, transactions, (err) ->
-        return done err if err
-
-        actual = runner.hooks.configuration.options.hookfiles
-        expected = [
-          pathStub.resolve(process.cwd(), 'foo/bar/hooks')
-          pathStub.resolve(process.cwd(), './test/fixtures/multifile/multifile_hooks.coffee')
-          pathStub.resolve(process.cwd(), './test/fixtures/test2_hooks.js')
-          pathStub.resolve(process.cwd(), './test/fixtures/test_hooks.coffee')
-        ]
-
-        assert.deepEqual actual, expected
-
         done()
 
-
     describe 'when files are valid js/coffeescript', () ->
       runner = null
       before () ->
diff --git a/test/unit/dredd-test.coffee b/test/unit/dredd-test.coffee
index 761c6286..c2ecb063 100644
--- a/test/unit/dredd-test.coffee
+++ b/test/unit/dredd-test.coffee
@@ -113,7 +113,7 @@ describe 'Dredd class', ->
           server: 'http://127.0.0.1:3000/'
           options:
             silent: true
-            path: ['./test/fixtures/multifile/*.apib', './test/fixtures/multifile/*.apib' ,'./test/fixtures/multifile/*.balony']
+            path: ['./test/fixtures/multifile/*.apib', './test/fixtures/multifile/*.apib']
         dredd = new Dredd(configuration)
 
       beforeEach ->
@@ -134,7 +134,6 @@ describe 'Dredd class', ->
         dredd.run (error) ->
           return done error if error
           assert.notInclude dredd.configuration.files, './test/fixtures/multifile/*.apib'
-          assert.notInclude dredd.configuration.files, './test/fixtures/multifile/*.balony'
           done()
 
       it 'should load file contents on paths to config', (done) ->
@@ -157,6 +156,28 @@ describe 'Dredd class', ->
           done()
 
 
+    describe 'when a glob pattern does not match any files and another does', ->
+      before ->
+        configuration =
+          server: 'http://127.0.0.1:3000/'
+          options:
+            silent: true
+            path: ['./test/fixtures/multifile/*.balony', './test/fixtures/multifile/*.apib']
+        dredd = new Dredd(configuration)
+
+      beforeEach ->
+        sinon.stub(dredd.runner, 'executeTransaction').callsFake (transaction, hooks, callback) ->
+          callback()
+
+      afterEach ->
+        dredd.runner.executeTransaction.restore()
+
+      it 'should return error', (done) ->
+        dredd.run (error) ->
+          assert.isOk error
+          done()
+
+
     describe 'when glob pattern does not match any files', ->
       before ->
         configuration =
diff --git a/test/unit/resolve-hookfiles-test.coffee b/test/unit/resolve-hookfiles-test.coffee
new file mode 100644
index 00000000..66a7f0e5
--- /dev/null
+++ b/test/unit/resolve-hookfiles-test.coffee
@@ -0,0 +1,111 @@
+path = require('path')
+{assert} = require('chai')
+
+resolveHookfiles = require('../../src/resolve-hookfiles')
+
+
+describe('resolveHookfiles()', ->
+  cwd = path.join(__filename, '..', '..', 'fixtures')
+
+  describe('when given no paths', ->
+    it('produces no results', ->
+      paths = resolveHookfiles([], cwd = cwd)
+      assert.deepEqual(paths, [])
+    )
+  )
+
+  describe('when given existing absolute filenames', ->
+    it('resolves them into absolute paths', ->
+      hookfiles = [
+        path.join(cwd, 'hooks.js'),
+        path.join(cwd, 'non-js-hooks.rb'),
+      ]
+      paths = resolveHookfiles(hookfiles, cwd = cwd)
+      assert.deepEqual(paths, hookfiles)
+    )
+  )
+
+  describe('when given existing relative filenames', ->
+    it('resolves them into absolute paths', ->
+      paths = resolveHookfiles(['./hooks.js', './non-js-hooks.rb'], cwd = cwd)
+      assert.deepEqual(paths, [
+        path.join(cwd, 'hooks.js'),
+        path.join(cwd, 'non-js-hooks.rb'),
+      ])
+    )
+  )
+
+  describe('when given non-existing filenames', ->
+    it('throws an error', ->
+      assert.throws( ->
+        resolveHookfiles(['./hooks.js', './foo/bar/42'], cwd = cwd)
+      , './foo/bar/42')
+    )
+  )
+
+  describe('when given glob pattern resolving to existing files', ->
+    it('resolves them into absolute paths', ->
+      paths = resolveHookfiles(['./**/hooks.js'], cwd = cwd)
+      assert.deepEqual(paths, [
+        path.join(cwd, 'hooks.js'),
+      ])
+    )
+  )
+
+  describe('when given glob pattern resolving to no files', ->
+    it('throws an error', ->
+      assert.throws( ->
+        resolveHookfiles(['./**/hooks.js', './**/foo/bar/foobar.js'], cwd = cwd)
+      , './**/foo/bar/foobar.js')
+    )
+  )
+
+  describe('when given both globs and filenames', ->
+    it('resolves them into absolute paths', ->
+      paths = resolveHookfiles(['./non-js-hooks.rb', './**/hooks.js'], cwd = cwd)
+      assert.deepEqual(paths, [
+        path.join(cwd, 'hooks.js'),
+        path.join(cwd, 'non-js-hooks.rb'),
+      ])
+    )
+
+    it('throws an error on non-existing filenams', ->
+      assert.throws( ->
+        resolveHookfiles(['./**/hooks.js', './foo/bar/42'], cwd = cwd)
+      , './foo/bar/42')
+    )
+
+    it('throws an error on globs resolving to no files', ->
+      assert.throws( ->
+        resolveHookfiles(['./hooks.js', './**/foo/bar/foobar.js'], cwd = cwd)
+      , './**/foo/bar/foobar.js')
+    )
+
+    it('returns the absolute paths alphabetically sorted', ->
+      paths = resolveHookfiles([
+        './**/*_hooks.*',
+        './hooks-glob/baz/x.js',
+        './hooks-glob/foo/y.js',
+        './hooks-glob/bar/z.js',
+        './hooks-glob/foo/a.js',
+        './hooks-glob/bar/b.js',
+        './hooks-glob/baz/c.js',
+        './hooks-glob/foo/o.js',
+        './hooks-glob/bar/p.js',
+      ], cwd = cwd)
+      assert.deepEqual(paths, [
+        path.join(cwd, 'hooks-glob/foo/a.js'),
+        path.join(cwd, 'hooks-glob/bar/b.js'),
+        path.join(cwd, 'hooks-glob/baz/c.js'),
+        path.join(cwd, 'multifile/multifile_hooks.coffee'),
+        path.join(cwd, 'hooks-glob/foo/o.js'),
+        path.join(cwd, 'hooks-glob/bar/p.js'),
+        path.join(cwd, 'test2_hooks.js'),
+        path.join(cwd, 'test_hooks.coffee'),
+        path.join(cwd, 'hooks-glob/baz/x.js'),
+        path.join(cwd, 'hooks-glob/foo/y.js'),
+        path.join(cwd, 'hooks-glob/bar/z.js'),
+      ])
+    )
+  )
+)