fix(wait-for-job): add catch block and emit error #749
Conversation
| @@ -289,6 +292,11 @@ export class Worker< | |||
| this.keys.active, | |||
| opts.drainDelay, | |||
| ); | |||
| } catch (error) { | |||
There was a problem hiding this comment.
don't really know how to make a test for this 🤔
There was a problem hiding this comment.
I think the only way to do it is by monkey patching client so that brpoplpush always throws an exception. So in the test overwrite this.blockingConnection.client with a stub. Sinon is the best for this, as we are already using it in other tests.
There was a problem hiding this comment.
so after adding the delay after if statement seems like in our test we are getting into that block, so I had to stub the delay method
| @@ -289,6 +292,11 @@ export class Worker< | |||
| this.keys.active, | |||
| opts.drainDelay, | |||
| ); | |||
| } catch (error) { | |||
There was a problem hiding this comment.
I think the only way to do it is by monkey patching client so that brpoplpush always throws an exception. So in the test overwrite this.blockingConnection.client with a stub. Sinon is the best for this, as we are already using it in other tests.
src/classes/worker.ts
Outdated
| @@ -289,6 +292,11 @@ export class Worker< | |||
| this.keys.active, | |||
| opts.drainDelay, | |||
| ); | |||
| } catch (error) { | |||
| if ((error as Error).message !== CONNECTION_CLOSED_ERROR_MSG) { | |||
| await delay(2000); | |||
There was a problem hiding this comment.
Just wondering, shouldn't we always delay? because otherwise if the error persists for a long time we will produce an endless loop here that may consume all CPU.
Also it would be great to refactor this magic number into a constant.
| @@ -323,7 +325,7 @@ describe('repeat', function() { | |||
| }); | |||
|
|
|||
| it('should remove repeated job when using removeOnComplete', async function() { | |||
| this.timeout(200000); | |||
| this.timeout(20000); | |||
There was a problem hiding this comment.
these changes are because we are adding a higher number but we could choose a lower one in those cases, even when these changes are going to fail because of a change, test should not get stopped for too much time
| } finally { | ||
| this.waiting = false; | ||
| } | ||
| return jobId; | ||
| } | ||
|
|
||
| async delay(): Promise<void> { |
There was a problem hiding this comment.
I need to do this in order to stub this function
There was a problem hiding this comment.
Ok, but we can keep it private.
There was a problem hiding this comment.
when it is prívate I cannot stub it :/
There was a problem hiding this comment.
aha, thats right. Then just a comment to explain why it is needed is enough.
roggervalf
force-pushed
the
fix/wait-for-job
branch
from
664f1ea
to
bf7923d
Compare
| @@ -150,7 +150,9 @@ export class RedisConnection extends EventEmitter { | |||
| await this._client.quit(); | |||
| } | |||
| } catch (error) { | |||
| handleError(error); | |||
| if (isNotConnectionError(error)) { | |||
There was a problem hiding this comment.
Nice change. It is always better to have explicit functions instead of "handle" which can be anything. 👍
## [1.46.7](v1.46.6...v1.46.7) (2021-09-16) ### Bug Fixes * **wait-for-job:** add catch block and emit error ([#749](#749)) ([b407f9a](b407f9a))
|
🎉 This PR is included in version 1.46.7 🎉 The release is available on: Your semantic-release bot 📦🚀 |
fixes #623, #36, #239