Timeout operation priority according to the source #3127
Conversation
nikocanvacom
force-pushed
the
niko-set-timeout-priorities-by-sources
branch
from
a2e1ad8
to
f879a76
Compare
lib/step.js
Outdated
| * @param {number} timeout - timeout in milliseconds or 0 if no timeout | ||
| * @param {number} source - 0 - test/suite, 1 - config force=true, 2 - code, 3 - config force=false | ||
| */ | ||
| this.setTotalTimeout = function (timeout, source = 3) { |
There was a problem hiding this comment.
source is not something that is really clear here.
should we call it priority?
There was a problem hiding this comment.
yep, we could call it priority. lower priority value gives higher priority to timeout value. I thought of calling it priority but I ended up calling it source as the logic here has very specific expectations for levels/priorities for timeout values from different sources and I tried to think of scenario where some other source of timeout try to meddle with this and couldn't.
There was a problem hiding this comment.
other plugins could also use this API to add their timeouts... So the priority is cleaner. But if lower value makes things more prioritized then let's call it order. In this case, 0 should be understandable to be the first value to pull out.
lib/plugin/stepTimeout.js
Outdated
| @@ -85,6 +85,6 @@ module.exports = (config) => { | |||
| } | |||
| } | |||
| stepTimeout = stepTimeout === undefined ? config.timeout : stepTimeout; | |||
| step.totalTimeout = stepTimeout * 1000; | |||
| step.setTotalTimeout(stepTimeout * 1000, config.force ? 1 : 3); | |||
There was a problem hiding this comment.
what does config.force do here?
I thought it will make global step timeouts more prioritized over local step timeouts. Does it?
There was a problem hiding this comment.
force makes config values to override values set from code I.limitTime(t)
There was a problem hiding this comment.
Ok, got it! But the name is very unclear... How about a more verbose name?
ignoreStepLimits: true
lib/step.js
Outdated
| new Map([...timeouts.entries()].sort().reverse()).forEach((timeout, order) => { | ||
| if (timeout !== undefined && ( | ||
| // when orders >= 10 - timeout value overrides those set with higher order elements | ||
| order >= 10 |
There was a problem hiding this comment.
this breaks my mind... Magical numbers 🧙 🔮
what the point of 10 number here?
Why not just the lower number is is more priority it gets?
lib/step.js
Outdated
| * @param {number} timeout - timeout in milliseconds or 0 if no timeout | ||
| * @param {number} order - <10 - test/suite, 10-19 - config when set to override timeouts from code, 20-29 - code, 30-39 - config | ||
| */ | ||
| this.setTimeout = function (timeout, order = 35) { |
There was a problem hiding this comment.
let's set 50 at the beginning
so it would be the lowest priority for now
35 is also magical number
There was a problem hiding this comment.
Could you have a look at the update in the last commit. It resolves the problem of really magical constant 10. Now the timeout override logic changes for orders below zero. Also as discussed introduced constants and jsdoc for them.
Motivation/Description of the PR
I.limitTimeand timeout set with configuration of stepTimeout plugin one from the code has priority unless stepTimeout.config.force===trueApplicable helpers:
Applicable plugins:
Type of change
Checklist:
npm run docs)npm run lint)npm test)