Do not initiate hooks if there are no hookfiles

[honzajavorek]

🚀 Why this change?

I believe it is a regression. @kylef had to work around it when upgrading Dredd in apiaryio/polls-api@4f66c34

📝 Related issues and Pull Requests

• Handle accept header without subtype polls-api#66

✅ What didn't I forget?

• To write docs
• To write tests
• To put Conventional Changelog prefixes in front of all my commits and run npm run lint

[honzajavorek]

Wait, it's wrong. if (!runner.configuration.hookfiles) { shouldn't work for an array in JavaScript... but I did it test-first, so I'm wondering how it can work 🤔

[artem-zakharchenko]

Please adjust the condition that checks if there are no hooks. Thanks.

[artem-zakharchenko]

You are right, this branch doesn't cover the scenario when there are no hooks. unner.configuration.hookfiles is an empty array [], which resolves to false in this condition.

The code follow down, and after:

let hookfiles;
  try {
    hookfiles = resolvePaths(runner.configuration.custom.cwd, runner.configuration.hookfiles);
  } catch (err) {
    return callback(err);
  }

// Override hookfiles option in configuration object with
// sorted and resolved files
runner.configuration.hookfiles = hookfiles;

After this you mutate the configuration.hookfiles with hookfiles. resolvePaths returns an empty array in case there were no hooks (empty array).

Probably, you could assert the length of hooks:

if (runner.configuration.hookfiles && runner.configuration.hookfiles.length ===0) {
  return callback(err)
}

[honzajavorek]

Yes, fixing the code is easy. Fixing the test to actually test something will be tricky.

[artem-zakharchenko]

Why to test on python?
It would spawn a hook handling process, if I'm not mistaken, but does it affect the behavior when no hooks are set?

If the behavior is different depending on whether it's javascript hooks or not, then we would need two tests respectively. But if it's the same, then maybe let's use the default options (javascript)?

[honzajavorek]

If the language is set, it's easier to spot the misbehaving. It's misbihaving in both cases (language set or nodejs hooks), but with language set the test fails even without the assert.

[honzajavorek]

...like this:

[artem-zakharchenko]

But failed attempt to execute not installed hooks handler means the hooks were attempted to run, isn't it?

[artem-zakharchenko]

The exception on the screenshot above means the code reached this point:

dredd/lib/addHooks.js

Lines 76 to 78 in 0da05fb

	// If other language than nodejs, run hooks worker client
	// Worker client will start the worker server and pass the "hookfiles" options as CLI arguments to it
	return (new HooksWorkerClient(runner)).start(callback);

Which, according to the issue, it shouldn't.

[honzajavorek]

Correct, because this has been "photographed" at the moment when I didn't have the assert in the test in place and the if wasn't in place in the implementation. So it reached the line you pointed out and failed with a try to find Python hooks. That's exactly what happened to @kylef. And that's exactly what shouldn't happen.

When the assert line is in place, it still fails with configuration not being undefined. With the if in place the test passes.

[honzajavorek]

I was first trying to find a failing test, which replicates the erroneous behavior. Then I added the if, which fixes it.

[honzajavorek]

@artem-zakharchenko Changed both the test and the code. I made sure the test fails without implementation. Without implementation if the if it actually fails even without the assert, with the same error @kylef reported (it tries to load Python hooks, because I've set the language).

[ApiaryBot]

🎉 This PR is included in version 11.1.4 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀