-
Notifications
You must be signed in to change notification settings - Fork 806
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
replace mocha and blanket with ava and nyc #506
Conversation
package.json
Outdated
"date.js": "~0.3.1", | ||
"debug": "^2.6.8", | ||
"debug": "~3.0.0", |
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.
You can get rid of these changes by rebasing with master git rebase origin/master
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 shouldn't be a problem since we squash the commits, right?
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.
Yup no problemo!
Can someone explain this test to me? Converted it to ava and now it fails and I think it's because ava is running faster than mocha but I'm not 100% sure. it('does not process locked jobs', done => {
const history = [];
jobs.define('lock job', {
lockLifetime: 300
}, (job, cb) => {
history.push(job.attrs.data.i);
setTimeout(() => {
cb();
}, 150);
});
jobs.start();
jobs.now('lock job', {i: 1});
jobs.now('lock job', {i: 2});
jobs.now('lock job', {i: 3});
setTimeout(() => {
expect(history).to.have.length(3);
expect(history).to.contain(1);
expect(history).to.contain(2);
expect(history).to.contain(3);
done();
}, 500);
}); |
Sorry but I don't really have insight into that test :-/ Maybe @lushc can figure out what's going on in here? In case pinging also @rschmukler |
If I can get help with a few of the integration tests then this should be done by the end of next week. |
I've got 113/123 tests passing so I've only got 10 left to finish off and I think we should be good to then review and merge.
|
package.json
Outdated
"moment-timezone": "^0.5.0", | ||
"mongodb": "^2.2.10" | ||
"moment-timezone": "~0.5.0", | ||
"mongodb": "~2.2.10" | ||
}, | ||
"devDependencies": { |
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.
With NPM 5.2.0 this was complaining about missing packages when running npm test
, so I had to:
npm i eslint-plugin-ava eslint-config-xo eslint-plugin-unicorn eslint-plugin-import --save-dev
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.
Looks like I wrote that review comment a month ago but forgot to submit it back then. :-) Not sure if that's relevant anymore.
test('returns nothing', t => { | ||
const {agenda} = t.context; | ||
|
||
t.is(agenda.cancel('jobA'), undefined); |
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.
Good question!
I feel that attempting to cancel a job that didn't exist kinda successfully fulfils the intention here, so it would not need to error? These are always tricky ones.
I remember seeing quite a few issues around cancelling jobs.
package.json
Outdated
"moment-timezone": "^0.5.0", | ||
"mongodb": "^2.2.10" | ||
"moment-timezone": "~0.5.0", | ||
"mongodb": "~2.2.10" |
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.
Remember to rebase with origin/master
as these are updated there.
const clearJobs = agenda => { | ||
return new Promise(resolve => { | ||
debug('jobs cleared'); | ||
agenda._mdb.collection('agendaJobs').remove({}, resolve); |
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.
Should we also clean up indices here?
Doesn't really matter for Travis but for local testing it might matter.
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.
We delete the whole database afterwards I was expecting that to trigger MongoDB's auto garbage collection for the indexes. Do we need to clean them ourselves?
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.
Collection.remove()
leaves them there, Collection.drop()
is what you'll need.
Although I'm not sure how Mongo garbage collection works exactly, that might handle them as well.
Related: #542 |
This is a WIP so please keep that in mind if you decide to review before I'm 100% finished.
Since there's no easy way to check the tests are evaluating the same before and after I'd highly suggest we get at least 2 people to review this.