make authz store tests deterministic in test coverage (!7438) · Merge requests · Warren Gifford / sourcegraph

Warren Gifford requested to merge store-test-deterministic into master Jan 02, 2020

Created by: sqs

Previously, the authz store tests were nondeterministic in their test coverage stats (eg see https://github.com/sourcegraph/sourcegraph/pull/7411#issuecomment-570105121). This is because sometimes loadedIDs would be empty and sometimes it would not, depending on whether GrantPendingPermissions was called before SetRepoPermissions. This commit fixes it so it is deterministic and improves the test so it tests all permutations of call ordering.

To debug this problem, I used the following diff:

diff --git a/enterprise/cmd/frontend/internal/authz/integration_test.go b/enterprise/cmd/frontend/internal/authz/integration_test.go
index 31f0bb0fb8..eb09051355 100644
--- a/enterprise/cmd/frontend/internal/authz/integration_test.go
+++ b/enterprise/cmd/frontend/internal/authz/integration_test.go
@@ -14,7 +14,7 @@ func TestIntegration(t *testing.T) {
                t.Skip()
        }
 
-       t.Parallel()
+       // t.Parallel()
 
        db, cleanup := dbtest.NewDB(t, *dsn)
        defer cleanup()
@@ -23,12 +23,12 @@ func TestIntegration(t *testing.T) {
                name string
                test func(*testing.T)
        }{
-               {"Store/LoadUserPermissions", testStoreLoadUserPermissions(db)},
-               {"Store/LoadRepoPermissions", testStoreLoadRepoPermissions(db)},
-               {"Store/SetRepoPermissions", testStoreSetRepoPermissions(db)},
-               {"Store/LoadUserPendingPermissions", testStoreLoadUserPendingPermissions(db)},
-               {"Store/SetRepoPendingPermissions", testStoreSetRepoPendingPermissions(db)},
-               {"Store/GrantPendingPermissions", testStoreGrantPendingPermissions(db)},
+               // {"Store/LoadUserPermissions", testStoreLoadUserPermissions(db)},
+               // {"Store/LoadRepoPermissions", testStoreLoadRepoPermissions(db)},
+               // {"Store/SetRepoPermissions", testStoreSetRepoPermissions(db)},
+               // {"Store/LoadUserPendingPermissions", testStoreLoadUserPendingPermissions(db)},
+               // {"Store/SetRepoPendingPermissions", testStoreSetRepoPendingPermissions(db)},
+               // {"Store/GrantPendingPermissions", testStoreGrantPendingPermissions(db)},
                {"Store/DatabaseDeadlocks", testStoreDatabaseDeadlocks(db)},
        } {
                t.Run(tc.name, tc.test)
diff --git a/enterprise/cmd/frontend/internal/authz/store.go b/enterprise/cmd/frontend/internal/authz/store.go
index 749f3f7f51..925759af2d 100644
--- a/enterprise/cmd/frontend/internal/authz/store.go
+++ b/enterprise/cmd/frontend/internal/authz/store.go
@@ -4,6 +4,7 @@ import (
        "context"
        "database/sql"
        "fmt"
+       "log"
        "strings"
        "time"
 
@@ -728,6 +729,7 @@ func (s *Store) GrantPendingPermissions(ctx context.Context, userID int32, p *Us
        if err != nil {
                return errors.Wrap(err, "batch load repo permissions")
        }
+       log.Printf("loadedIDs %d %v, ids %v", len(loadedIDs), loadedIDs, ids)
 
        updatedAt := txs.clock()
        updatedPerms := make([]*RepoPermissions, 0, len(ids))

and then ran:

$ while true; do PGPASSWORD=q PGDATABASE=sourcegraph go test -cover ./enterprise/cmd/frontend/internal/authz/ -v -count 1 -coverprofile /tmp/cover.out.1 && sort -o /tmp/cover.out.1 /tmp/cover.out.1 && sort -o /tmp/cover.out.2 /tmp/cover.out.2 && diff -u /tmp/cover.out.{1,2} || break; sleep 1; date; done

An example output that showed nondeterminism here is:

=== RUN   TestIntegration
=== RUN   TestIntegration/Store/DatabaseDeadlocks
2020/01/01 19:50:13 loadedIDs 0 map[], ids [1]
2020/01/01 19:50:13 loadedIDs 1 map[1:{1}], ids [1]
2020/01/01 19:50:13 loadedIDs 1 map[1:{1}], ids [1]
2020/01/01 19:50:13 loadedIDs 1 map[1:{1}], ids [1]
2020/01/01 19:50:13 loadedIDs 1 map[1:{1}], ids [1]
2020/01/01 19:50:13 loadedIDs 1 map[1:{1}], ids [1]
2020/01/01 19:50:13 loadedIDs 1 map[1:{1}], ids [1]
2020/01/01 19:50:13 loadedIDs 1 map[1:{1}], ids [1]
2020/01/01 19:50:13 loadedIDs 1 map[1:{1}], ids [1]
2020/01/01 19:50:13 loadedIDs 1 map[1:{1}], ids [1]
2020/01/01 19:50:13 loadedIDs 1 map[1:{1}], ids [1]
2020/01/01 19:50:13 loadedIDs 1 map[1:{1}], ids [1]
2020/01/01 19:50:13 loadedIDs 1 map[1:{1}], ids [1]
2020/01/01 19:50:13 loadedIDs 1 map[1:{1}], ids [1]
2020/01/01 19:50:13 loadedIDs 1 map[1:{1}], ids [1]
2020/01/01 19:50:13 loadedIDs 1 map[1:{1}], ids [1]
2020/01/01 19:50:13 loadedIDs 1 map[1:{1}], ids [1]
2020/01/01 19:50:13 loadedIDs 1 map[1:{1}], ids [1]
2020/01/01 19:50:13 loadedIDs 1 map[1:{1}], ids [1]
2020/01/01 19:50:13 loadedIDs 1 map[1:{1}], ids [1]
2020/01/01 19:50:13 loadedIDs 1 map[1:{1}], ids [1]
2020/01/01 19:50:13 loadedIDs 1 map[1:{1}], ids [1]
2020/01/01 19:50:13 loadedIDs 1 map[1:{1}], ids [1]
2020/01/01 19:50:13 loadedIDs 1 map[1:{1}], ids [1]
2020/01/01 19:50:13 loadedIDs 1 map[1:{1}], ids [1]
2020/01/01 19:50:13 loadedIDs 1 map[1:{1}], ids [1]
2020/01/01 19:50:13 loadedIDs 1 map[1:{1}], ids [1]
2020/01/01 19:50:13 loadedIDs 1 map[1:{1}], ids [1]
2020/01/01 19:50:13 loadedIDs 1 map[1:{1}], ids [1]
2020/01/01 19:50:13 loadedIDs 1 map[1:{1}], ids [1]
2020/01/01 19:50:13 loadedIDs 1 map[1:{1}], ids [1]
2020/01/01 19:50:13 loadedIDs 1 map[1:{1}], ids [1]
2020/01/01 19:50:13 loadedIDs 1 map[1:{1}], ids [1]
2020/01/01 19:50:13 loadedIDs 1 map[1:{1}], ids [1]
2020/01/01 19:50:13 loadedIDs 1 map[1:{1}], ids [1]
2020/01/01 19:50:13 loadedIDs 1 map[1:{1}], ids [1]
2020/01/01 19:50:13 loadedIDs 1 map[1:{1}], ids [1]
2020/01/01 19:50:13 loadedIDs 1 map[1:{1}], ids [1]
2020/01/01 19:50:13 loadedIDs 1 map[1:{1}], ids [1]
2020/01/01 19:50:13 loadedIDs 1 map[1:{1}], ids [1]
2020/01/01 19:50:13 loadedIDs 1 map[1:{1}], ids [1]
2020/01/01 19:50:13 loadedIDs 1 map[1:{1}], ids [1]
--- PASS: TestIntegration (2.18s)
    --- PASS: TestIntegration/Store/DatabaseDeadlocks (0.20s)
PASS
coverage: 69.4% of statements
ok  	github.com/sourcegraph/sourcegraph/enterprise/cmd/frontend/internal/authz	2.202s	coverage: 69.4% of statements
--- /tmp/cover.out.1	2020-01-01 19:50:13.723853385 -0800
+++ /tmp/cover.out.2	2020-01-01 19:50:13.727853498 -0800
@@ -195,7 +195,7 @@
 github.com/sourcegraph/sourcegraph/enterprise/cmd/frontend/internal/authz/store.go:729.16,731.3 1 0
 github.com/sourcegraph/sourcegraph/enterprise/cmd/frontend/internal/authz/store.go:732.2,736.21 4 1
 github.com/sourcegraph/sourcegraph/enterprise/cmd/frontend/internal/authz/store.go:736.21,739.20 3 1
-github.com/sourcegraph/sourcegraph/enterprise/cmd/frontend/internal/authz/store.go:739.20,741.4 1 1
+github.com/sourcegraph/sourcegraph/enterprise/cmd/frontend/internal/authz/store.go:739.20,741.4 1 0
 github.com/sourcegraph/sourcegraph/enterprise/cmd/frontend/internal/authz/store.go:743.3,750.5 2 1
 github.com/sourcegraph/sourcegraph/enterprise/cmd/frontend/internal/authz/store.go:753.2,753.75 1 1
 github.com/sourcegraph/sourcegraph/enterprise/cmd/frontend/internal/authz/store.go:753.75,755.3 1 0

make authz store tests deterministic in test coverage

Merge request reports