Reviewed 5 of 5 files at r1.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @chaehni)

go/hidden_path_srv/internal/hiddenpathdb/adapter/pathdb_adapter.go, line 47 at r1 (raw file):

// If no database exists a new database is created. If the schema version of the
// stored database is different from the one in schema.go, an error is returned.
func New(path string) (*PathDBAdapter, error) {

No we shouldn't instantiate a specific type of pathdb. Instead accept a given implementation it will make it much more flexible, we can then pass any implementation of the pathdb interface, even a mock would be possible.

func New(pdb pathdb.PathDB) *PathDBAdapter {
	return &PathDBAdapter{
		backend:    pdb,
		readWriter: &readWriter{pdb},
	}
}

go/hidden_path_srv/internal/hiddenpathdb/adapter/pathdb_adapter.go, line 62 at r1 (raw file):

		queryParams = &query.Params{
			HpCfgIDs: convertIds(params.GroupIds),
			EndsAt:   []addr.IA{params.EndsAt},

Hm so this will only work for down segments. I guess for now that is fine.

go/hidden_path_srv/internal/hiddenpathdb/adapter/pathdb_adapter_test.go, line 91 at r1 (raw file):

// TestOpenExisting tests that New does not overwrite an existing database if
// versions match.
func TestOpenExisting(t *testing.T) {

This will not be needed once you construct directly with pathdb interface.

go/hidden_path_srv/internal/hiddenpathdb/adapter/pathdb_adapter_test.go, line 110 at r1 (raw file):

}

func TestGet(t *testing.T) {

It would also be possible to have all tests as a single table test:

func TestAdapter(t *testing.T) {
	tests := map[string]func(*testing.T, context.Context, *mock_pathdb.MockPathDB){
		"Get with params": func(t *testing.T, ctx context.Context, pdb *mock_pathdb.MockPathDB) {
			pdb.EXPECT().Get(ctx, queryParams)
			New(pdb).Get(ctx, params)
		},
	}
	for name, test := range tests {
		t.Run(name, func(t *testing.T) {
			ctrl := gomock.NewController(t)
			defer ctrl.Finish()
			ctx := context.Background()
			pdb := mock_pathdb.NewMockPathDB(ctrl)
			test(t, ctx, pdb)
		})
	}
}

this way we only have the setup code, which is the same for all tests, once.

Reviewable status: 3 of 5 files reviewed, 3 unresolved discussions (waiting on @lukedirtwalker)

go/hidden_path_srv/internal/hiddenpathdb/adapter/pathdb_adapter.go, line 47 at r1 (raw file):

Done.

go/hidden_path_srv/internal/hiddenpathdb/adapter/pathdb_adapter_test.go, line 91 at r1 (raw file):

Done.

go/hidden_path_srv/internal/hiddenpathdb/adapter/pathdb_adapter_test.go, line 110 at r1 (raw file):

Done.

Reviewed 2 of 2 files at r2, 1 of 1 files at r3.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @chaehni)

go/hidden_path_srv/internal/hiddenpathdb/adapter/pathdb_adapter_test.go, line 40 at r2 (raw file):

	suffix   = uint16(1337)
	ifs      = []uint64{0, 5, 2, 3, 6, 3, 1, 0}
	groupIds = hpsegreq.GroupIdSet{

Note this makes the tests non-deterministic. Since the order is arbitrary. You need to implement matchers for groupId slices.

go/hidden_path_srv/internal/hiddenpathdb/adapter/pathdb_adapter_test.go, line 126 at r2 (raw file):

}

func TestTransaction(t *testing.T) {

Commit and Rollback could also be merged into the previous function.

		"Commit": func(t *testing.T, ctx context.Context, pdb *mock_pathdb.MockPathDB,
			ctrl *gomock.Controller) {

			tx := mock_pathdb.NewMockTransaction(ctrl)
			pdb.EXPECT().BeginTransaction(ctx, gomock.Any()).Return(tx, nil)
			tx.EXPECT().Commit()
			pdbTx, err := New(pdb).BeginTransaction(ctx, nil)
			require.NoError(t, err)
			pdbTx.Commit()
		},

This way we no longer have to use the private API of the adapter.

Reviewed 3 of 3 files at r4.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @chaehni)

go/hidden_path_srv/internal/hiddenpathdb/adapter/pathdb_adapter_test.go, line 86 at r4 (raw file):

			pseg, _ := pathdbtest.AllocPathSegment(t, ctrl, ifs, uint32(10))
			pdb.EXPECT().InsertWithHPCfgIDs(ctx, getSeg(pseg), matchers.EqHPCfgIDs(hpCfgIDs))
			fmt.Println("want")

Remove please

Reviewable status: 5 of 6 files reviewed, 1 unresolved discussion (waiting on @lukedirtwalker)

go/hidden_path_srv/internal/hiddenpathdb/adapter/pathdb_adapter_test.go, line 40 at r2 (raw file):

Done.

go/hidden_path_srv/internal/hiddenpathdb/adapter/pathdb_adapter_test.go, line 126 at r2 (raw file):

Neat! Didn't know about the Return() function. I also changed the package to adapter_test

go/hidden_path_srv/internal/hiddenpathdb/adapter/pathdb_adapter_test.go, line 86 at r4 (raw file):

Done.

Reviewed 1 of 1 files at r5.
Reviewable status: complete! all files reviewed, all discussions resolved