-
-
Notifications
You must be signed in to change notification settings - Fork 325
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.
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
Fix: forever on command line #360
Conversation
Co-authored-by: Simone Busoli <simone.busoli@gmail.com>
@@ -11,7 +11,7 @@ const isValidFn = (opt) => (!opt || typeof opt === 'function' || typeof opt === | |||
|
|||
const lessThanOneError = (label) => new Error(`${label} can not be less than 1`) | |||
const greaterThanZeroError = (label) => new Error(`${label} must be greater than 0`) | |||
const minIfPresent = (val, min) => val && val < min | |||
const minIfPresent = (val, min) => val !== null && val < min |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This used to return 0
if val
was 0
which resulted in a falsy result when it should be true.
if (/[a-zA-Z]/.exec(opts.duration)) { | ||
try { | ||
opts.duration = timestring(opts.duration) | ||
} catch (error) { | ||
return error | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If timestring
was invalid then the error was being swallowed. It is now returned as per other errors.
// if forever is true then a promise is not returned and we need to try ... catch errors | ||
try { | ||
const tracker = initJob(argv) | ||
if (tracker.then) { | ||
tracker.catch((err) => { | ||
console.error(err.message) | ||
}) | ||
} | ||
} catch (err) { | ||
console.error(err.message) | ||
}) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The actual fix for forever
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, a couple of comments about the approach though
@@ -239,9 +239,17 @@ function start (argv) { | |||
}) | |||
}) | |||
} else { | |||
initJob(argv).catch((err) => { | |||
// if forever is true then a promise is not returned and we need to try ... catch errors |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: if rather than handling this in a special way we made the forever code path return a promise too, so that we don't have to handle special cases here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was my original approach but I ran into problems with the reporting. I just tried again now and it is working 😕 . Would you like me to investigate further or shall I merge as is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Up to you. My personal preference would be to do it "properly", but I'm not a maintainer of the repo either so I leave that to you and the maintainers.
@@ -45,7 +45,7 @@ | |||
"sinon": "^10.0.1", | |||
"split2": "^3.2.2", | |||
"standard": "^16.0.3", | |||
"tap": "^14.10.8" | |||
"tap": "^15.0.2" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
considering that 90% of the changes are tap-related, you could have considered keeping the tap upgrade out of this change and do it standalone instead
bump, can this go in? |
This PR started out to fix the forever bug on the command line. fixes #358
However, it was difficult to unit test the fix using the installed version of tap which required upgrading to tap 15.
tap 15 introduced a new problem, in that the coverage test is stricter than it used to be and as a result total coverage has fallen from the previous 100% to 96% (lines). I have examined all the uncovered code and added additional tests for all the low lying fruit. What remains is difficult to test without refactoring. Much of the current coverage is achieved through functional tests that happen to cover imported functions rather than unit tests and the odd lines not being tested really require unit tests being created for the whole function rather than just the odd fix to create coverage.
I've added minimum coverage values toI've had to lower the coverage further as node 10 skips some tests and this isn't taken into account in the coverage report.taprc
to maintain at least the coverage that we currently have.Upgrading tap to v 15 also produced a bunch of deprecation warnings for
t.tearDown
,t.deepEqual
,t.is
andt.false
. These have all been updated to their current variants.Finally a couple of bugs were found in
validate.js
whilst writing tests for it. I've noted them in the code below.