- Notifications
You must be signed in to change notification settings - Fork927
chore: improve notifications tests#13863
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Also add some more comments to improve readabilitySigned-off-by: Danny Kopping <danny@coder.com>
Signed-off-by: Danny Kopping <danny@coder.com>
Signed-off-by: Danny Kopping <danny@coder.com>
| if !dbtestutil.WillUsePostgres() { | ||
| t.Skip("This test requires postgres") | ||
| t.Skip("This test requires postgres; it relies on business-logic implemented in the database") |
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.
nit: implementedonly
| } | ||
| ctx, logger, db := setup(t) | ||
| // SETUP | ||
| ctx, logger, db := setupInMemory(t) |
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.
The problem with this approach is that we are not testing against the real database. I'm convinced if it is a truly safe way to go. Which parts of the implementation are missing in dbmem?
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.
dbmem doesn't actually perform the bulk updates, and along with that comes the retry logic; nor does it handle leases or locking.
There still are some tests which test the real database such asTestBackpressure,TestRetries, andTestExpiredLeaseIsRequeued. Each of those test the full scope of enqueue -> dequeue -> update -> requeue -> update.
In this test specifically, the database is not required to be tested - provided the observed behaviour of having a notification be dispatched to a webhook endpoint is achieved; so it's safe to use a mock database.
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.
What's the pro of no longer using Postgres for these tests? I'm unsure of how may PostgreSQL tests remain, but I'd feel a bit uncomfortable if all our testing was in dbmem.
Otherwise changes look good 👍🏻
Signed-off-by: Danny Kopping <danny@coder.com>
dannykopping commentedJul 10, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
The main benefit of not using Postgres really is just speed. See also#13863 (comment) for further explanation about the tests still using the database. It's also useful to have a ~functioning |
…include state syncSigned-off-by: Danny Kopping <danny@coder.com>
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 to go 👍
542fff7 intomainUh oh!
There was an error while loading.Please reload this page.
Blocks#13799