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 flexible test setup #118

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions pkg/conn/gorilla/gorilla.go
Original file line number Diff line number Diff line change
Expand Up @@ -239,6 +239,8 @@ func (ws *WebSocket) initialize() {
var res rpc.RPCResponse
err := ws.read(&res)
if err != nil {
// TODO: Handle error correctly, otherwise this can panic
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// TODO: Handle error correctly, otherwise this can panic
// TODO(gh-119): Handle error correctly, otherwise this can panic

Copy link
Author

Choose a reason for hiding this comment

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

Added with 69ef565 (used the hash syntax instead, which becomes a link in some editor setup)

// after 1000 tries in a tight loop.
ws.logger.Error(err.Error())
continue
}
Expand Down
90 changes: 90 additions & 0 deletions query_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
package surrealdb

import (
"testing"

"github.com/google/go-cmp/cmp"
"github.com/surrealdb/surrealdb.go/pkg/marshal"
)

// TestSurrealDBLocal runs a local SurrealDB instance by calling
// `NewTestSurrealDB(t)`, which is a dedicated SurrealDB instance with no data.
func TestSurrealDBLocal(t *testing.T) {
t.Parallel()
cases := map[string]struct {
schema []string
dbInteraction func(*testing.T, *DB)
keepServerUponError bool
}{
"first case": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could the case names describe what is being tested in each case please?

Copy link
Author

Choose a reason for hiding this comment

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

This was meant to be so that each test case to use CREATE keyword, which cannot run against the same table in parallel. Probably I should have named this test func so that it is clearly about table tests can run in parallel -- I had no intention of adding too many test cases in this particular test func.

schema: []string{
"CREATE user:x SET name = 'xxxx'",
"CREATE user:y SET name = 'yyyy'",
},
dbInteraction: func(t *testing.T, db *DB) {
data, err := db.Select("user")
if err != nil {
t.Fatalf("failed to select, %v", err)
}
type dataholder struct {
ID string `json:"id,omitempty"`
Name string `json:"name,omitempty"`
}
d, err := marshal.SmartUnmarshal[dataholder](data, nil)
if err != nil {
t.Fatal(err)
}
want := []dataholder{
{ID: "user:x", Name: "xxxx"},
{ID: "user:y", Name: "yyyy"},
}
if diff := cmp.Diff(want, d); diff != "" {
t.Errorf("Result mismatch (-want +got):\n%s", diff)
}
},
},
"second case": {
schema: []string{
"CREATE user:x SET name = 'xxxx'",
"CREATE user:y SET name = 'yyyy'",
},
dbInteraction: func(t *testing.T, db *DB) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing interaction.

Overall, the interaction should be part of the test instead of a parameter. Currently, this test contains several cases and only shares a setup. The setup could be a separate function, so the cases don't need to be nested.

Copy link
Author

Choose a reason for hiding this comment

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

This was indeed intentional -- you can have extra interaction, or not. It would be probably cleaner to write more tests in a separate func, though. I'll look into that.

},
},
"third case": {
schema: []string{
"CREATE user:x SET name = 'xxxx'",
"CREATE user:y SET name = 'yyyy'",
"CREATE user:y SET name = 'yyyy'", // Should fail
Copy link
Contributor

Choose a reason for hiding this comment

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

We can test this, but it is worth remembering that the code we are testing in the driver is the driver code, not the DB implementation. We assume the db behaves correctly.

We can, however, verify the error for this.

Copy link
Author

Choose a reason for hiding this comment

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

We can, however, verify the error for this.

Yes, absolutely, this is the point I was going to add -- that would mean I need a separate test check for failure. At that point, I wouldn't be able to use the Prepare method above (because it runs t.Fatal), and would probably need to use db.Query instead.

I'll update the table definition accordingly.

},
dbInteraction: func(t *testing.T, db *DB) {
},
},
}

for name, tc := range cases {
tc := tc
t.Run(name, func(t *testing.T) {
t.Parallel()

endpoint, db, cancel := NewTestSurrealDB(t)
defer func() {
// If test case failed, keep the server running for further
// investigation. This could be something we can turn on with
// some special flag.
if tc.keepServerUponError && t.Failed() {
return
}
// Otherwise cleanup the server.
cancel()
}()
_ = endpoint // Not used for this test.

for _, s := range tc.schema {
db.Prepare(t, s)
}

tc.dbInteraction(t, db.DB)
})
}
}
Loading