Nv 2826 investigate failing digest tests #4140
Conversation
NV-2826 Investigate failing digest tests
What?The Notion doc for the investigation: https://www.notion.so/novuhq/Investigate-failing-digest-tests-33bba615327d4ca3a815a76b9dbf5ef4?pvs=4 Why? (Context)Definition of Done |
| @@ -69,6 +69,9 @@ export class DigestFilterSteps { | |||
| const keys = path.split('.'); | |||
|
|
|||
| for (const key of keys) { | |||
| if (result === undefined) { | |||
There was a problem hiding this comment.
Fix so if you have a nested digestkey but do not pass a nested value it will throw "can not find x on undefined"
| @@ -448,71 +446,6 @@ describe('Trigger event - Digest triggered events - /v1/events/trigger (POST)', | |||
| expect(cancelledDigestJobs.length).to.eql(1); | |||
| }); | |||
|
|
|||
| xit('should be able to update existing message on the in-app digest', async function () { | |||
There was a problem hiding this comment.
Removed those tests, as this functionality doesn't exists anymore
| expect(digests[1].payload.postId).not.to.equal(digests[2].payload.postId); | ||
| expect(digests[3].payload.postId).to.equal(undefined); | ||
| expect(digests[1].payload.postId).to.equal(digests[4].payload.postId); | ||
| const noPostIdJobs = digests.filter((job) => !job.payload.postId); |
There was a problem hiding this comment.
Not to rely on hardcoded values based on index which can be flaky
There was a problem hiding this comment.
Another option would have been the query make it stricter in the conditions and sort the entities by creation date.
| @@ -102,7 +102,7 @@ describe('Trigger event - Digest triggered events - /v1/events/trigger (POST)', | |||
|
|
|||
| expect(delayedJob).to.be.ok; | |||
|
|
|||
| await session.awaitRunningJobs(template?._id, false, 1); | |||
| await session.awaitRunningJobs(template?._id, false, 0); | |||
There was a problem hiding this comment.
Could it be that these were failing because the delay time was changed and the unfinished jobs argument of awaitRunningJobs wasn't updated?
There was a problem hiding this comment.
yes and now when they are changed to merged it should be 0 as changed to here.
| @@ -335,10 +333,10 @@ describe('Trigger event - Digest triggered events - /v1/events/trigger (POST)', | |||
| const firstDigestKeyBatch = messages.filter((message) => (message.content as string).includes('Hello world 2')); | |||
| const secondDigestKeyBatch = messages.filter((message) => (message.content as string).includes('Hello world 1')); | |||
|
|
|||
| expect(firstDigestKeyBatch.length).to.eql(2); | |||
| expect(firstDigestKeyBatch.length).to.eql(1); | |||
There was a problem hiding this comment.
With the change of status MERGED we will only create a message per digest?
There was a problem hiding this comment.
Yes, i think it's also the expected behaviour. Since only one message should be sent to the end user. Wdyt?
There was a problem hiding this comment.
I am just thinking in a potential situation where the digested messages content are different. In this test we are assuming that the content is the same but I think we would need to think in covering the scenario that I mention. Even if right now might not be totally possible as the content of the message is quite static.
Example:
Digesting GitHub notifications:
- X has mentioned in PR-1
- Y has mentioned in PR-2.
- Z has mentioned in PR-3.
Would become one single notification of the 3 messages. So being the content different we might need to keep them 3.
| expect(digests[1].payload.postId).not.to.equal(digests[2].payload.postId); | ||
| expect(digests[3].payload.postId).to.equal(undefined); | ||
| expect(digests[1].payload.postId).to.equal(digests[4].payload.postId); | ||
| const noPostIdJobs = digests.filter((job) => !job.payload.postId); |
There was a problem hiding this comment.
Another option would have been the query make it stricter in the conditions and sort the entities by creation date.
| @@ -858,7 +678,6 @@ describe('Trigger event - Digest triggered events - /v1/events/trigger (POST)', | |||
| expect(jobCount).to.equal(15); | |||
| }); | |||
|
|
|||
| // TODO: Review backoff individually | |||
| it.skip('should create multiple digest based on different digestKeys with backoff', async function () { | |||
There was a problem hiding this comment.
We couldn't fix this one?
There was a problem hiding this comment.
We identified the race conditions on the backoff part functionality, due to the size of the change will address it later in another PR.
| @@ -6,4 +6,5 @@ export enum JobStatusEnum { | |||
| FAILED = 'failed', | |||
| DELAYED = 'delayed', | |||
| CANCELED = 'canceled', | |||
| MERGED = 'merged', | |||
| await this.jobRepository.update( | ||
| { | ||
| _environmentId: job._environmentId, | ||
| _id: job._id, | ||
| }, | ||
| { | ||
| $set: { | ||
| status: JobStatusEnum.MERGED, | ||
| }, | ||
| } | ||
| ); | ||
|
|
||
| await this.jobRepository.updateAllChildJobStatus( | ||
| job, | ||
| JobStatusEnum.MERGED | ||
| ); |
There was a problem hiding this comment.
This should happen before digestMergedExecutionDetails as it would be misleading that in Execution Details we have that update and for any reason these 2 calls failed.
Also have you considered keep these 2 updates together in a single functionality? Where we could have another functionality that would need update job status from a parent job, for a status different to merged?
| @@ -69,6 +69,9 @@ export class DigestFilterSteps { | |||
| const keys = path.split('.'); | |||
|
|
|||
| for (const key of keys) { | |||
| if (result === undefined) { | |||
No description provided.