Skip to content
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

chore(datastore): fix for flaky integration tests #6979

Merged
merged 10 commits into from
Nov 7, 2022
51 changes: 36 additions & 15 deletions datastore/integration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"flag"
"fmt"
"log"
"math"
"os"
"reflect"
"sort"
Expand Down Expand Up @@ -199,29 +200,49 @@ func TestIntegration_Basics(t *testing.T) {
}

func TestIntegration_GetWithReadTime(t *testing.T) {
ctx, _ := context.WithTimeout(context.Background(), time.Second*20)
ctx, cancel := context.WithTimeout(context.Background(), time.Second*20)
client := newTestClient(ctx, t)
defer cancel()
defer client.Close()

type X struct {
I int
S string
T time.Time
U interface{}
type RT struct {
TimeCreated time.Time
}

x0 := X{66, "99", timeNow.Truncate(time.Millisecond), "X"}
k, err := client.Put(ctx, IncompleteKey("BasicsX", nil), &x0)
rt1 := RT{time.Now()}
k := NameKey("RT", "ReadTime", nil)

tx, err := client.NewTransaction(ctx)
if err != nil {
t.Fatalf("client.Put: %v", err)
t.Fatal(err)
}
x1 := X{}
client.WithReadOptions(ReadTime(time.Now()))
err = client.Get(ctx, k, &x1)
if err != nil {
t.Errorf("client.Get: %v", err)

if _, err := tx.Put(k, &rt1); err != nil {
t.Fatalf("Transaction.Put: %v\n", err)
}

if _, err := tx.Commit(); err != nil {
t.Fatalf("Transaction.Commit: %v\n", err)
}

// Wait a little bit for eventual consistency to catch up.
time.Sleep(time.Duration(10 * math.Pow(10, 9)))
Copy link
Member

Choose a reason for hiding this comment

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

Is it really worth waiting 10s every time? I think this is only worth it if it's guaranteed to take that long, otherwise might as well try right away, or after a shorter wait.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried several different time lengths from 1-10 seconds to ensure consistent test success. 10s was the lowest value that seemed to work every time (locally).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You know what, it looks like the issue was that I didn't hook into the retries correctly :). Nice catch!


testutil.Retry(t, 5, time.Duration(10*math.Pow(10, 9)), func(r *testutil.R) {
got := RT{}
tm := ReadTime(time.Now())

client.WithReadOptions(tm)

// If the Entity isn't available at the requested read time, we get
// a "datastore: no such entity" error. The ReadTime is otherwise not
// exposed in anyway in the response.
err = client.Get(ctx, k, &got)
if err != nil {
t.Errorf("client.Get: %v", err)
Copy link
Member

Choose a reason for hiding this comment

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

Should this be r.Errorf to ensure the retry works correctly?

Btw, have you observed the retries working?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Nice catch!

}
})

// Cleanup
_ = client.Delete(ctx, k)
}
Expand Down Expand Up @@ -321,7 +342,7 @@ func TestIntegration_GetMulti(t *testing.T) {
t.Error(err)
}

err := client.WithReadOptions(ReadTime(time.Now())).GetMulti(ctx, dstKeys, dst)
err := client.GetMulti(ctx, dstKeys, dst)
if err == nil {
t.Errorf("client.GetMulti got %v, expected error", err)
}
Expand Down