Add test for configurable port

[ddelnano]

This is to address #11.

I already have this working for the php hooks based on a dredd change I made to pass the port to the hooks server command.

Todo

• Figure out what flag should be used for the port (php hooks already use --port, see here)
• Update all hook server implementations with fix
• Figure out rollout plan.

I am assuming my dredd PR will need some work since it was the quickest change just to see this test work locally on my machine. The change I made will cause problems with backwards compatibility (for instance @w-vi mentioned that the python hooks treat all arguments as filenames). So atleast just wanted to get this rolling and hear feedback. Another flag that is not used but can be provided is the --hooks-worker-handler-host so maybe we should include that change in this PR as well.

[honzajavorek]

I like the proposed change to the test suite, i.e. to test whether hook handlers are accepting host and port options. I have no objections to the names, --host= and --port= are okay with me, pretty much straightforward.

As you mentioned, we need to find out how other hook libs will behave if they're given --port and --host before releasing this or similar change. People can have older installations while upgrading Dredd itself. I'm not sure whether other hook handlers will at least ignore the unknown arguments 🤔 (see #11 (comment)). Maybe hook handlers could somehow expose an information about whether they accept the option or not? Maybe hardcoded version check? Maybe just warning on Dredd side that this is a new option and it's up to the hook handler to comply (with suggestion to upgrade the hook handler to the latest version), would be enough. Any other ideas?

We have one existing and passing implementation (in PHP), so I think this is good to be merged to the test suite (reminder: we should document the process of accepting and propagating changes to this test suite at least in the README 🤔 ). Once this is merged, hook handler authors should be notified in their issues that their project is lacking behind the spec. This is non-trivial change, so I'm far from sending ready-to-be-merged Pull Requests everywhere, as I usually strive to do. I'd leave catching up to the hook handler authors. The behavior is currently broken anyway, so if a hook handler won't catch up, it won't break anything more than is already broken.

What do you think? @ddelnano @gonzalo-bulnes @w-vi @netmilk

[w-vi]

Maybe a simple check sending an event with a new name like features to get the feature list would solve it. If it does not reply correctly than it is old and is not supporting this? That would definitely work for python hooks.

[ddelnano]

@w-vi I like the idea of Dredd be able to discover the feature set of the hooks server at runtime. I am not familiar with Dredd internals enough to understand how easy / hard the change would be to support that but certainly something I would not mind helping out with.

@honzajavorek also just want to point out the php hooks are "passing" in the sense that as long as Dredd implements to some extent what I did in this. If people started to implement this test they would need to pull that PR I linked to.

[honzajavorek]

If people started to implement this test they would need to pull that PR I linked to.

Yes, that's good to have in mind. We need to think this through before the Dredd PR gets merged, but then the PR needs to be merged before this one 😅

[ddelnano]

@gonzalo-bulnes @w-vi @netmilk @honzajavorek I outlined the impact of existing hooks servers here.

[ungrim97]

s/php/{{mylanguage}}/

[ddelnano]

👍 updated.