Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Do not initiate hooks if there are no hookfiles #1374

Merged
merged 1 commit into from
May 22, 2019
Merged

Do not initiate hooks if there are no hookfiles #1374

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions lib/addHooks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,11 @@ module.exports = function addHooks(runner, transactions, callback) {
runner.hooks.transactions[transaction.name] = transaction;
});

// No hooks
if (!runner.configuration.hookfiles || !runner.configuration.hookfiles.length) {
return callback();
}

// Loading hookfiles from fs
let hookfiles;
try {
Expand Down
14 changes: 14 additions & 0 deletions test/unit/addHooks-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,9 @@ describe('addHooks()', () => {

it('sets transactionRunner.hooks.configuation', (done) => {
const transactionRunner = createTransactionRunner();
transactionRunner.configuration.hookfiles = [
'./hooks.js',
];

addHooks(transactionRunner, [], (err) => {
assert.deepEqual(
Expand All @@ -83,4 +86,15 @@ describe('addHooks()', () => {
done(err);
});
});

it('skips hooks loading when there are no hookfiles', (done) => {
const transactionRunner = createTransactionRunner();
transactionRunner.configuration.hookfiles = [];
transactionRunner.configuration.language = 'python';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...like this:

image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


addHooks(transactionRunner, [], (err) => {
assert.isUndefined(transactionRunner.hooks.configuration);
done(err);
});
});
});