Skip to content

make authz store tests deterministic in test coverage

Administrator requested to merge store-test-deterministic into master

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

Merge request reports