-
Notifications
You must be signed in to change notification settings - Fork 802
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
Fixes for failing test cases #852
Conversation
…priority to guarantee priority is respected among jobs scheduled for the same date
…times the test fails due to an imprecission of a milisecond which is an acceptable imprecission
…ven though we do not delete jobs on agend.stop()
|
||
agenda._collection.findOne({name: 'longRunningJob'}, (err, job) => { |
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.
I suspect there was an await missing here, that caused the afterEach mocha section of the test to be executed clearing the jobs which eventually made the findOne return undefined, no job found.
…ault behavior in mocha
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 is beautiful and I regret not reviewing it earlier! <3
Thanks @koresar for the review as well!
These modifications try to fix an issue with failing test cases which are preventing contributions to make it to the repo (when these contributions have nothing to do with the tests themselves). The changes are: * new queuing mechanism since the previous didn't guarantee priority when executing jobs scheduled the same datetime (agenda uses setTiemout and sometimes the job with lowest priority was executed, though not processed, first). Test case involved: job concurrency -> 'should run jobs as first in first out (FIFO) with respect to priority' * On repeatEvery -> 'sets the nextRunAt property with skipImmediate' test case we expect precise timing in milliseconds and that's not always the with the processing we do to set repeatEvery, sometimes we schedule the job one millisecond after the expected time, I find this acceptable and changed the test case to expect something in the range of 3 minutes sharp and 3 minutes + 2 milliseconds for nextRunAt. * start/stop -> 'clears locks on stop' fails on many PRs not being able to find 'longRunningJob' on db. I provided the param job on define callback since not having params has caused trouble before and increased jobTimeout when we execute the tests on TravisCI (actually I only set the TRAVIS env var on .travis.yml, the tests were already prepared to increase this timeout when that env var is present). Hopefully it'll help, never suffered the issue when testing locally, I've only seen it on TravisCI.
These modifications try to fix an issue with failing test cases which are preventing contributions to make it to the repo (when these contributinos have nothing to do with the tests themselves).
The changes are:
new queuing mechanism since the previous didn't guarantee priority when executing jobs scheduled the same datetime (agenda uses setTiemout and sometimes the job with lowest priority was executed, though not processed, first). Test case involved: job concurrency -> 'should run jobs as first in first out (FIFO) with respect to priority'
On repeatEvery -> 'sets the nextRunAt property with skipImmediate' test case we expect precise timing in miliseconds and that's not always the with the processing we do to set repeatEvery, sometimes we schedule the job one milisecond after the expected time, I find this acceptable and changed the test case to expect something in the range of 3 minutes sharp and 3 minutes + 2 miliseconds for nextRunAt.
start/stop -> 'clears locks on stop' fails on many PRs not being able to find 'longRunningJob' on db. I provided the param job on define callback since not having params has caused trouble before and increased jobTimeout when we execute the tests on TravisCI (actually I only set the TRAVIS env var on .travis.yml, the tests were already prepared to increase this timeout when that env var is present). Hopefully it'll help, never suffered the issue when testing locally, I've only seen it on TravisCI.