Skip to content

chore: add webhook tests for notification subsystem #13942

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

Merged
merged 4 commits into from
Jul 19, 2024

Conversation

dannykopping
Copy link
Contributor

Signed-off-by: Danny Kopping <danny@coder.com>
@dannykopping dannykopping requested review from mtojek and mafredri and removed request for mtojek July 19, 2024 09:12
@dannykopping dannykopping requested a review from mtojek July 19, 2024 09:13
Signed-off-by: Danny Kopping <danny@coder.com>
@dannykopping dannykopping force-pushed the dk/system-notifications-webhooks branch from 506a7e9 to 07b4a69 Compare July 19, 2024 09:18
Copy link
Member

@mafredri mafredri left a comment

Choose a reason for hiding this comment

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

I'd like to see all goroutine requires changed to assert, but other than that, LGTM!

expectSuccess: false,
expectRetryable: true,
expectErr: "non-2xx response (500)",
},
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we could add a test that sends bad data (wrong format) too, just to have the case covered?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not actually possible, the webhook dispatch sends a well-formed request.

See func (w *WebhookHandler) dispatch

Signed-off-by: Danny Kopping <danny@coder.com>
Signed-off-by: Danny Kopping <danny@coder.com>
@dannykopping dannykopping merged commit 492ab1c into main Jul 19, 2024
29 checks passed
@dannykopping dannykopping deleted the dk/system-notifications-webhooks branch July 19, 2024 10:03
@github-actions github-actions bot locked and limited conversation to collaborators Jul 19, 2024
Copy link
Member

@mtojek mtojek left a comment

Choose a reason for hiding this comment

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

post-merge approval

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add detailed unit tests for SMTP & webhook notification dispatchers
3 participants