Skip to content
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

test(job-scheduler): add tests with drain and promote for job scheduler #2944

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

fgozdz
Copy link
Contributor

@fgozdz fgozdz commented Nov 28, 2024

ref #247 #2405

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@fgozdz fgozdz requested a review from manast November 28, 2024 09:47
@roggervalf roggervalf changed the title feat(job-scheduler): tests with drain and promote for job scheduler test(job-scheduler): add tests with drain and promote for job scheduler Nov 29, 2024

await queue.promoteJobs();

expect(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about

const delayedCount = await queue.getDelayedCount();
const waitingCount = await queue.getWaitingCount();

expect(delayedCount).to.be.equal(1);
expect(waitingCount).to.be.equal(1);

by separating each expectation will be easily to identify where something is failing

expect(
(await queue.getDelayedCount()) == 1 ||
(await queue.getWaitingCount()) == 1 ||
(await queue.getActiveCount()) == 1,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as worker is having a no-operational processor, it could process a waiting job really fast and no be in active state, I recommend removing worker instance and this check


await queue.upsertJobScheduler('scheduler-test', {
every: 50000,
immediately: true,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this option is not needed when using every option as it happens by default

(await queue.getWaitingCount()) == 1 ||
(await queue.getActiveCount()) == 1,
).to.be.true;
await queue.removeJobScheduler('scheduler-test');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

may no be needed as afterEach hook is removing all keys for this queue

every: 50000,
immediately: true,
});
const worker = new Worker(queueName, async () => {}, { connection });
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe initialize worker after drain operation as worker could take the first iteration as quick as possible

it('worker should start processing repeatable jobs after drain', async function () {
await queue.upsertJobScheduler('scheduler-test', {
every: 50000,
immediately: true,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

immediately happens by default when using every option, no need to pass it

immediately: true,
});

expect(worker.isRunning()).to.be.true;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of this check, you are looking for checking that worker is able to process jobs from scheduler after drain operation, you can use a queueEvent instance to listen for completed state or from worker instance like in https://github.com/taskforcesh/bullmq/blob/master/tests/test_worker.ts#L1835-L1851

await queue.promoteJobs();

const waitingCount = await queue.getWaitingCount();
expect(waitingCount).to.be.equal(1);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to check that at least the next repeatable was scheduled you may need to check the delayed count as well, so after this line you can add:

const delayedCount = await queue.getDeleayedCount();
expect(delayedCount).to.be.equal(1);

});
});

const job = await queue.add('test', { foo: 'bar' });
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this addition looks like it's not needed but you may need to check the completion of the scheduled job from scheduler

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants