Skip to content
This repository has been archived by the owner on Nov 8, 2024. It is now read-only.

Tests do not validate hooks can listen on different ports #11

Open
ddelnano opened this issue Jan 21, 2017 · 3 comments · May be fixed by #81
Open

Tests do not validate hooks can listen on different ports #11

ddelnano opened this issue Jan 21, 2017 · 3 comments · May be fixed by #81

Comments

@ddelnano
Copy link

I fixed this bug a while back with the php hooks (Go is probably an offender at the moment). Nothing in the test suite validates that a hook server's implementation will work with dredd's --hooks-worker-handler-port. Making this as a reminder to myself to make a future PR to add this test. Do we have any idea if other hook servers respect this? I am assuming ruby and python are good since you guys developed them. That would leave the Go and Perl ones potentially if thats the case.

@honzajavorek
Copy link
Contributor

That's true, this should be fixed. The --hooks-worker-handler-port was introduced much later than the test suite 😕

Do we have any idea if other hook servers respect this?

I don't know. I'm not even sure about the reference ones (Ruby, Python), when you're bringing it up. It needs to be verified. cc @gonzalo-bulnes @w-vi

@w-vi
Copy link
Contributor

w-vi commented Jan 23, 2017

As for Python hooks passing options has dire consequences, they are actually treated as a hook file as any argument is treated as hook file. Clearly a bug.

@gonzalo-bulnes
Copy link
Contributor

gonzalo-bulnes commented Jan 23, 2017

The Ruby hooks use an hardcoded host and port at this point. It shouldn't be too difficult to provide configurable ones.

I must say I looked at this Dredd option in a very narrow way, thinking it was intended to allow Dredd to use non-standard hooks handlers. But I see no reason to not provide additional flexibility on the handlers side : )

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants