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

Add an integration test for the test server #115

Merged
merged 1 commit into from
Oct 18, 2021
Merged

Conversation

josephschorr
Copy link
Member

No description provided.

@github-actions github-actions bot added the area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools) label Sep 22, 2021
@josephschorr josephschorr force-pushed the testserver-test branch 4 times, most recently from 580c980 to 8ef4f33 Compare September 22, 2021 20:59
@josephschorr josephschorr marked this pull request as ready for review September 22, 2021 21:02
// Give the service time to boot.
iterationCount := 0
for {
iterationCount++
Copy link
Member

Choose a reason for hiding this comment

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

let's use require.Eventually here too

Copy link
Member Author

Choose a reason for hiding this comment

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

ooo. Very cool

require.NoError(t, err)
require.Equal(t, v1.CheckPermissionResponse_PERMISSIONSHIP_HAS_PERMISSION, v1Resp.Permissionship)

// Ensure check against readonly works as well.
Copy link
Member

Choose a reason for hiding this comment

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

add a check that a write against readonly fails

@josephschorr
Copy link
Member Author

Updated

cmd/spicedb/testserver_integration_test.go Show resolved Hide resolved
@@ -0,0 +1,212 @@
//go:build withcontainerimage
// +build withcontainerimage
Copy link
Member

Choose a reason for hiding this comment

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

can we standardize on a build tag? maybe replace this and ci with docker since that's what this requires

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll change to docker, but leaving CI as is

- uses: "actions/setup-go@v2"
with:
go-version: "^1.17"
- name: "Test with image"
Copy link
Member

Choose a reason for hiding this comment

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

We maybe want to repurpose this whole job to be for e2e tests and eventually have evan's e2e tests run in here, too


func TestTestServer(t *testing.T) {
tester, err := newTester(t,
&dockertest.RunOptions{
Copy link
Member

Choose a reason for hiding this comment

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

It'd be great if this could build the image, too so that devs can't accidentally run these tests with stale images.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd have to wire up the Docker API calls too; I think we should just move newTester into a util package, export it, and then optionally add a "build it not present" flag to it

},
},
},
})
Copy link
Member

Choose a reason for hiding this comment

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

style nitpick that i think makes things more readable

_, err = rov0client.Write(context.Background(), &v0.WriteRequest{
	Updates: []*v0.RelationTupleUpdate{{  // <---- 
		Operation: v0.RelationTupleUpdate_CREATE,
		Tuple: &v0.RelationTuple{
			ObjectAndRelation: &v0.ObjectAndRelation{
				Namespace: "resource",
				ObjectId:  "someresource",
				Relation:  "reader",
			},
			User: &v0.User{UserOneof: &v0.User_Userset{Userset: &v0.ObjectAndRelation{
				Namespace: "user",
				ObjectId:  "somegal",
				Relation:  "...",
			}}},
		},
	}},
})

Copy link
Member Author

Choose a reason for hiding this comment

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

I actually disagree; I'd wonder how I'm adding a single item directly to the slice

Copy link
Member

Choose a reason for hiding this comment

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

well, this is how it's done in many places in the codebase, so we should pick one

Updates: []*v0.RelationTupleUpdate{
{
Operation: v0.RelationTupleUpdate_CREATE,
Tuple: &v0.RelationTuple{
Copy link
Member

Choose a reason for hiding this comment

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

can't we declare a variable for this tuple? we're reusing it here
that'd even let us use tuple.ToRelationship in the other test

cleanup func()
}

func newTester(t *testing.T, containerOpts *dockertest.RunOptions) (*spicedbHandle, error) {
Copy link
Member

Choose a reason for hiding this comment

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

I really don't like this function name or design even as we've used it in the other tests.

It doesn't have the container details in it, but it does have the knowledge that the container will somehow be SpiceDB because of the healthcheck built into it.

Now that we've copied and pasted this code a few places, can we make it more generic?

Copy link
Member Author

Choose a reason for hiding this comment

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

Personally, I want it into a util package that SpiceDB exports

&dockertest.RunOptions{
Repository: "authzed/spicedb",
Tag: "latest",
Cmd: []string{"spicedb", "serve-testing", "--log-level", "debug"},
Copy link
Contributor

Choose a reason for hiding this comment

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

What about moving this into the e2e package from #75? the test would build/run much faster, and avoid the need for build tags (plus that e2e GHA already handles caching).

Copy link
Member Author

Choose a reason for hiding this comment

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

I explicitly want this tested against the built binary image, as that's how people are going to be using it most of the time. Unless I'm mistaken, #75 doesn't run the server as a distinct binary?

Copy link
Contributor

Choose a reason for hiding this comment

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

It does, it execs the built spicedb

@josephschorr
Copy link
Member Author

Updated, but still using Docker. Will issue a followup commit to change to use the e2e stuff once in

Signed-off-by: Joseph Schorr <josephschorr@users.noreply.github.com>
@jzelinskie jzelinskie dismissed their stale review October 18, 2021 20:05

I'd rather this go in now and get cleaned up later than have the branch stagnate.

@josephschorr josephschorr merged commit 56f3feb into main Oct 18, 2021
@josephschorr josephschorr deleted the testserver-test branch October 18, 2021 20:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants