chore: improve notifications tests #13863
Conversation
Also add some more comments to improve readability Signed-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") |
| } | ||
| ctx, logger, db := setup(t) | ||
| // SETUP | ||
| ctx, logger, db := setupInMemory(t) |
There was a problem hiding this comment.
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.
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 as TestBackpressure, TestRetries, and TestExpiredLeaseIsRequeued. 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.
Signed-off-by: Danny Kopping <danny@coder.com>
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 sync Signed-off-by: Danny Kopping <danny@coder.com>
Blocks #13799