PROPOSAL: Allow user to specify verification timeout #31
Conversation
|
Eh, I'd probably not allow the user to set the timeout since the purpose of
it is to figure out if pact started or not. This whole thing of running a
separate process is a pain to manage. We need to find another way to
figure out if the provider verifier started, maybe through log checks.
After that, the verification process can take as long as the user needs.
I'm currently on vacation at the moment, but you're welcome to make that
change if you want. I'll verify it and you can then release.
…On Wed, Feb 22, 2017, 11:11 PM Matt Fellows ***@***.***> wrote:
@mefellows <https://github.com/mefellows> requested your review on:
pact-foundation/pact-js-core#31
<#31> PROPOSAL: Allow
user to specify verification timeout.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#31 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAjA5PfIrYyTCi5j3x6oQtrdBgCooII6ks5rfTEggaJpZM4MJn0v>
.
|
|
Actually, there is already a check for if it fails to start: this._instance.once('close', function (code) {
code == 0 ? deferred.resolve(output) : deferred.reject(new Error(output));
});This is basically just a timeout for the verification process itself. I think we have two choices:
|
|
The close event is different, that's for when the process has completed and
exits. The other is a check that the process actually started. Check the
mock server side on how it's done there. I think we just need to rejig
things a bit. We still might need a timeout for the verification process
though. In most unit testing libraries, it's 10 seconds. We could always
increment it and add an option to set a default one.
What does the Ruby version do? Does it timeout? We should probably try to
mimic it as much as possible to reduce confusion.
…On Thu, Feb 23, 2017, 1:34 PM Matt Fellows ***@***.***> wrote:
Actually, there is already a check
<https://github.com/pact-foundation/pact-node/pull/31/files#diff-13a26c29d16405f6e694991a87084025L76>
for if it fails to start.
this._instance.once('close', function (code) {
code == 0 ? deferred.resolve(output) : deferred.reject(new Error(output));
});
This is basically just a timeout for the *verification* process itself. I
think we have two choices:
1. Don't impose a timeout at all (just return the promise)
2. Impose a timeout and allow the user to change it (return the
promise with a timeout attached)
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#31 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAjA5KE78gUBZekrQFsVHO1WM0aIiDP6ks5rfftPgaJpZM4MJn0v>
.
|
|
This isn't like the mock server though, it doesn't start a service on any port that we can monitor - it just invokes our service in sequence as per the pact file. All we can monitor is an active PID, which we get via Can you think of anything else I'm missing? |
|
Is there a log that's written out when started? We could just listen for
that to know the process started alright.
If that seems too intricate, just remove the timeout on the process and add
it to the verification step with a default of 30 seconds that can be
modified by the user.
…On Sat, Feb 25, 2017, 4:38 AM Matt Fellows ***@***.***> wrote:
This isn't like the mock server though, it doesn't start a service on any
port that we can monitor - it just invokes *our* service in sequence as
per the pact file. All we can monitor is an active PID, which we get via
child_process. I've tested directly modifying the verifier (make it fail
early, throw errors, move the file etc.) and all cases are caught by this
close event, with the appropriate error bubbling back up.
Can you think of anything else I'm missing?
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#31 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAjA5OnjC3sY6nSyovOf-UwxLatq_w-iks5rgCDbgaJpZM4MJn0v>
.
|
|
No logs I'm afraid.
I'm not following, sorry. Are you saying rather than have it on |
|
Ah right, the verifying step is all on the same command line. Then yeah,
let's make the timeout on the process changeable and defaults to 30s.
…On Sat, Feb 25, 2017, 5:27 PM Matt Fellows ***@***.***> wrote:
No logs I'm afraid.
remove the timeout on the process and add it to the verification step
I'm not following, sorry. Are you saying rather than have it on
Verifier.prototype.verify (verifier.js) put it in verifyPacts()
(pact.js)? Or something else?
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#31 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAjA5IR9Hd8bs-_unTkAWza3OMsJXAftks5rgNUGgaJpZM4MJn0v>
.
|
mefellows
force-pushed
the
fix/issue-30
branch
from
a68b8d1 to
cfb780d
Compare
|
Done - modified from 60s to 30s. Good to see you enjoy your holidays the way I do :) |
src/verifier.js
Outdated
| if (options.timeout) { | ||
| checkTypes.assert.positive(options.timeout); | ||
| } else { | ||
| options.timeout = 30000; |
There was a problem hiding this comment.
Could you move this up to the start of the function and have the default there like options.timeout = options.timeout || 30000. Leave the conditional/assert there though.
|
Is there an ETA on this being merged? We have run into an issue verifying pacts in Vagrant where the hard coded 10s timeout is not long enough. Modifying verifier.js and bumping the timeout to 20s solves the timeout issues for us. |
|
It will happen tonight :)
…On Mon, Feb 27, 2017 at 3:24 PM, Will Falconer ***@***.***> wrote:
Is there an ETA on this being merged?
We have run into an issue verifying pacts in Vagrant where the hard coded
10s timeout is not long enough. Modifying verifier.js and bumping the
timeout to 20s solves the timeout issues for us.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#31 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AADSjGNwjxCohftiZuufegiZpSk4E_heks5rglACgaJpZM4MJn0v>
.
--
Matt Fellows
- http://www.onegeek.com.au/
|
|
@mefellows merged. Please increment minor version, tag as the same, then push changes with tag to deploy :) |
|
@scrumtech |
Fixes #30, but please count this as a proposal rather than a full-blown PR. I haven't fully explored test coverage etc. at this point.
Adds ability for a user to specify a timeout on the verification process. Previously there was a 10s verification timeout.
Another alternative could be to remove the timeout altogether and simply allow the parent testing framework to perform the timeout.
Looking for your thoughts @mboudreau.