Closed ryancormack closed 1 year ago
Thanks for putting this together so quickly 🚀
It looks great, I've only left a couple of minor comments (plus an important comment to extract the new sleepBetweenRunsMs
input parameter from the event
object).
Thank you, this looks great 🚀 I might add a few more tests to make sure that the sleep is actually performed.
In the meantime, I'd suggest deploying your fork and testing it with a real downstream system to make sure it works as intended.
I might add a few more tests to make sure that the sleep is actually performed.
Thanks for this, I was unsure how to test that, but it's something I'd be really interested to see how to do it, or if you point me in the right direction I'll try and pick it up myself.
I've run the changed/updated executor in my account and it does as expected, with and without a sleep, so it seems to be working. Thanks
@ryancormack to keep things simple I ended up moving the sleep
function to utils.js
, so we can simply mock it when testing the Executor logic, and then I used Sinon Fake Times to make sure we don't actually have to wait for X ms.
@ryancormack apologies for the burst of commits :) I messed up with the debugger.
The current implementation and tests should work 👌
I've haven't properly tested it myself, but will try to do it tomorrow.
Please feel free to share any additional feedback, in the meantime.
@alexcasalboni thanks for this. I've had a look through and it makes sense. I'll deploy these changes tomorrow and give them a run
It seems to run fine, and as expected, with and without the new delay 👍🏻
@ryancormack apologies for the delay in merging this 🙏
AWS re:Invent is coming soon and I'm planning to merge all open PRs between Dec 12th and Dec 22nd 🎄
@ryancormack or we could just say that sleepBetweenRunsMs
makes sense only if parallelInvocation=false
:)
If we implement sleep between runs for runInParallel
, it becomes runInSeries
.
Is there even a good reason to run everything in parallel with sleepBetweenRunsMs
? I can't think of it.
Thank you @ryancormack 🙏 I've applied a couple of minor changes and synced with the latest master branch (which now includes GH Actions instead of Travis CI).
I'm going to merge this and publish a new version on SAR asap :)
v4.2.2
is now published on SAR 🚀
Thanks for your help with this Alex 👍🏻
Fixes #179