TOOLS-4075 Add integration test for restoring TTL collMod oplog#891
TOOLS-4075 Add integration test for restoring TTL collMod oplog#891
collMod oplog#891Conversation
nickweinberger
left a comment
nickweinberger
left a comment
There was a problem hiding this comment.
Core logic from the restore test looks good, left a couple questions partly for my own understanding and partly as suggestions. Approving % addressing these questions at your discretion.
| err = session.Database(testDB).Collection(testCollName).Drop(ctx) | ||
| require.NoError(t, err) | ||
|
|
||
| err = session.Database(testDB).CreateCollection(ctx, testCollName) |
There was a problem hiding this comment.
Why is it necessary to drop and create the collection?
| defer failpoint.Reset() | ||
|
|
||
| go func() { | ||
| convertIndexToTTL(ctx, t) |
There was a problem hiding this comment.
Know you're following existing patterns here but I think calling testify's require (which convertIndexToTTL does) within a goroutine is bad practice in go. I think on failure it only exits the goroutine but not the whole test, so the test keeps running and it can cause weird panics that are harder to debug. Up to you whether you want to address or just note as tech debt.
| if cmp, err := testutil.CompareFCV(fcv, "6.0"); err != nil || cmp < 0 { | ||
| if err != nil { | ||
| t.Errorf("error getting FCV: %v", err) | ||
| } | ||
| t.Skipf("Requires server with FCV 6.0 or later; found %v", fcv) |
There was a problem hiding this comment.
Why exactly 6.0+ required? Maybe useful to add into the error.
There was a problem hiding this comment.
Would also ideally put this in a helper for reusability, but I haven't worked much w/ the tools testing so don't know if that's worth it.
| } | ||
| } | ||
|
|
||
| func TestOplogDumpCollModTTL(t *testing.T) { |
There was a problem hiding this comment.
Is this test intended as a generator for the restore test's testdata fixtures (like TestOplogDumpBypassDocumentValidation) ? If so, might benefit from a comment. Otherwise, mind explaining just what this test is doing?
| if cmp, err := testutil.CompareFCV(fcv, "6.0"); err != nil || cmp < 0 { | ||
| if err != nil { | ||
| t.Errorf("error getting FCV: %v", err) | ||
| } | ||
| t.Skipf("Requires server with FCV 6.0 or later; found %v", fcv) | ||
| } |
There was a problem hiding this comment.
Same comment about helper.
2 participants
This commit adds integration tests for verifying that mongodump+mongorestore handles a
collModoplog that converts a non-TTL index into TTL correctly. This was a bug fixed in TOOLS-4071.