Adds tests for snapshot atomicity and iteration #28
Conversation
…s into dylan/snapshot-iterator
…t-connector-postgres into dylan/teardown
…or-postgres into dylan/test-iteration
dylanlott
force-pushed
the
dylan/teardown
branch
from
27ba7c9 to
b544e42
Compare
…or-postgres into dylan/test-iteration
|
Tests are looking nice already! 👍 |
…s into dylan/test-iteration
…s into dylan/test-iteration
lovromazgon
left a comment
lovromazgon
left a comment
There was a problem hiding this comment.
Approving already to not block this further, but I'd appreciate it if subtests were split into normal tests. Looking good otherwise 👍
source/logrepl/snapshot_test.go
Outdated
| now := time.Now() | ||
| rec, err := s.Next(ctx) | ||
| is.NoErr(err) | ||
| func TestSnapshot(t *testing.T) { |
There was a problem hiding this comment.
The three subtest in this test have nothing in common except the connection pool, IMO we should keep it simple and split them back into regular tests.
There was a problem hiding this comment.
I slightly disagree, they all test constraints of the snapshot's lifecycle, but I factored them back out to their own tests.
There was a problem hiding this comment.
To clarify what I meant - they have no setup or cleanup in common, they are pretty much independent of each other, that's why I think they should be separate tests (that also makes it easier to run each one separately). The fact that they are in this file already indicates that they test the snapshot iterator, no need to further group IMO. I would probably go with a subtest if there were some substantial setup/cleanup that we don't want to repeat for each test, or for table-driven tests.
| rec, err := s.Next(ctx) | ||
| is.NoErr(err) | ||
| func TestSnapshot(t *testing.T) { | ||
| is := is.New(t) |
There was a problem hiding this comment.
Sidenote: watch out, using the same is instance in subtests means that once the first subtest fails the whole test will stop and no other subtests will even be run. It would be better to call is.New for each subtest. That said, this won't be a problem if we split the subtests into separate tests.
There was a problem hiding this comment.
That's good to know 👍 Thanks!
Description
Adds tests for snapshot atomicity and iteration.
Quick checks: