Add flexible test setup #118

rytswd · 2023-12-01T23:02:27Z

WHAT

This adds a new function func NewTestSurrealDB(t testing.TB) (string, *DBForTest, func()) for test code.

By checking the PATH with exec.LookPath for surreal CLI, we can assume that the real SurrealDB process can be spawned for each test case. This does not need any extra setup other than surreal CLI, and simply running go test ./... would create as many SurrealDB instances necessary locally.

WHY

There will be many test cases where we want to have more complicated queries and interactions. With the existing test suite setup, SurrealDB instance is carefully managed to ensure each test to run cleanly, but it has quite a bit of learning curve. With this new function, there could be as many test cases as necessary, and we can easily define table driven test cases for various test cases.

HOW

As an example, the simplest way to start a test SurrealDB instance is as follows.

func TestSimpleTestSurrealDB(t *testing.T) {
	_, db, cancel := NewTestSurrealDB(t)
	defer cancel()

	// If you want to add some seed data, you can use Prepare method.
	db.Prepare(t, "CREATE user:x SET name = 'x'")

	// ... More test steps ...
}

The Prepare command is an extra helper method to add some test data prior to the test. Its implementation is rather crude, but it would be helpful to have a separate function like this with testing.TB, so that anyone new to the code would see this as test setup before the actual test details.

Other Notes

I didn't want to touch any of the existing test cases yet, as that would be a massive change
When surreal CLI is not available in PATH, it would simply skip the test cases (with some logging)
It may require some adjustment for CI/CD setup?
I would suggest adding direnv setup so that anyone who opens up this repository will get SurrealDB CLI added to their PATH (as long as they use Nix + direnv)
I considered creating a separate package, so that other packages can refer to for their testing (similar to https://pkg.go.dev/cloud.google.com/go/spanner/spannertest), but kept it for this repo for now
I hit many websocket impl related panic during my local testing, and left a comment on that

ElecTwix · 2023-12-03T11:01:56Z

Hi, this looks nice and is good for a disclaimer I also think we should recommend and enforce most test cases on the suite for consistency.

I hit many websocket impl related panic during my local testing, and left a comment on that

I previously investigated this issue and ultimately realized that the panic was expected and the github issues have pointed out this was intentional it meant to wake the developer up for something wrong.
They recommended that after repeated errors create another websocket.
I was already working on it but I'm a little busy lately, I will look into it next week.

ref
#110

rytswd · 2023-12-03T14:41:37Z

Hi, this looks nice and is good for a disclaimer I also think we should recommend and enforce most test cases on the suite for consistency.

Sorry I'm not sure what you mean. The current test setup is not easy to digest, and each test case should be as isolated as possible so that it is easy to add/remove test, and test cases will then become documentation by themselves. In that sense, I was hoping this test setup would help streamline the test cases in the future, potentially entirely stripping out the need of suite setup and cleanup, and any assumptions made for contribution. Are you on the same page as I am, or are you suggesting something else?

I previously investigated this issue and ultimately realized that the panic was expected and the github issues have pointed out this was intentional it meant to wake the developer up for something wrong.

The referred code from gorilla package is about its behaviour to panic so that users (in our case it's surrealdb.go) would need to handle the error correctly.

If you read the function docstring, it is clear that this is an implementation error in the websocket handling code.
https://github.com/gorilla/websocket/blob/main/conn.go#L1017-L1020

// Applications must break out of the application's read loop when this method
// returns a non-nil error value. Errors returned from this method are
// permanent. Once this method returns a non-nil error, all subsequent calls to
// this method return the same error.

The tight loop is a result of applications handling error and not terminating the connection, which would lead to the underlying websocket connection to be reused. This is clearly an implementation error, and we should tackle that correctly.

ElecTwix · 2023-12-03T16:58:38Z

I apologize for the lack of clarity in my previous message. I have just returned from Devfest, and my trip was quite lengthy.

From my understanding, we are creating our test suite for SurrealDB.

With the existing test suite setup, SurrealDB instance is carefully managed to ensure each test to run cleanly, but it has quite a bit of learning curve.

Although I acknowledge that Suite can make it hard for people to comprehend and contribute, I am unsure how the proposed solution of introducing another custom suite would resolve this problem.

We can divide testify files into more manageable chunks that are easier for developers to understand and contribute to.

With this new function, there could be as many test cases as necessary, and we can easily define table driven test cases for various test cases

table-driven test cases are a good point, we can implement this it can be useful.

The Prepare command is an extra helper method to add some test data prior to the test. Its implementation is rather crude, but it would be helpful to have a separate function like this with testing.TB, so that anyone new to the code would see this as test setup before the actual test details.

This can be beneficial for reusability and simplicity, but I believe we should implement it in testify.

Conclusion

Although PR has made some valid points, I believe that we should not implement our own suite to solve this issue. Instead, I believe that we should use Testify for this case.

About Gorilla

The referred code from the gorilla package

I wanted to clarify that I was simply making an observation. I understand that this is not PR's area of focus, but I wanted to highlight the issue because of the changes that have been made.

The referred code from gorilla package is about its behaviour to panic so that users (in our case it's surrealdb.go) would need to handle the error correctly.

Yes, we must handle the error correctly. Additionally, I would like to point out that in some cases, when Gorilla encounters a timeout error, it cannot recover. In such cases, we should also reconnect the websocket.

The tight loop is a result of applications handling error and not terminating the connection, which would lead to the underlying websocket connection to be reused. This is an implementation error, and we should tackle that correctly.

I am not sure if you are referring to SurrealDB's Go driver implementation or the Gorilla. If we are talking about SurrealDB's Go driver, then you are correct.

For context:
gorilla/websocket#657
gorilla/websocket#474

rytswd · 2023-12-04T01:34:31Z

I have not mentioned a single point about "testify", and while I admit I'm not versed with testify way of writing test cases, that is really not the point I'm trying to make.

My code here is to add a utility function for test (and thus the file is called util_for_test.go), and can be used in any test environment -- as I'd imagine testify would still use testing.T or testing.B under the hood (if that assumption is incorrect, then we can simply create another NewTestSurrealDBTestify).

Let me put some more examples about why we want flexible test setup, because that seems to be completely misunderstood from your comment:

With the current test setup (regardless of testify used or not), it assumes there is SurrealDB server running at localhost:8000
With the current test setup (regardless of testify used or not), we cannot run tests in parallel because it uses a single instance
With the current test setup (regardless of testify used or not), there is table clean-up to take place for "user" table but nothing else -- and regardless of the use cases, these need to be tightly handled
If we want to conduct a lengthy test with more complex dataset, the test setup and cleanup would become more complicated, which would make it difficult to read the actual test cases

The baseline is that, there should be no assumption made, and any contributor should feel that it is easy to contribute by knowing that there will be a robust set of test cases, that are easy to understand and extend / adjust as necessary.

I can write more test cases to further illustrate what I mean by this, and indeed, table driven test with t.Parallel() is something that's possible with this utility. I actually had a test case that I was playing with in my local, so just pushed with f9ce745 to help clarify what I mean by it. (go.mod change for adding go-cmp is not included, so this is just for illustration purpose for now)

Some responses to your comments

We can divide testify files into more manageable chunks that are easier for developers to understand and contribute to.

I don't think this would help. I'm not talking about the file structure, and it's more about the overall feel of how tests are managed.

Although PR has made some valid points, I believe that we should not implement our own suite to solve this issue. Instead, I believe that we should use Testify for this case.

It is not clear to me why we would want to use Testify over other solutions. Is it just that because we use it already? That argument sounds very weak, and I'd like to understand how Testify is really the right solution. (Disclaimer: I've never used any test tools with Go, except when the OSS package already uses something. I just think Go standard test support is rich, and idiomatic.)

About Gorilla

As it's certainly a digress from the main points, I just agree that "we must handle the error correctly", and we aren't at the moment. I'm not talking about the third party solution which we don't control indeed.

rytswd · 2023-12-06T12:27:09Z

util_for_test.go

+func Test_NewTestSurrealDB_Simple(t *testing.T) {
+	// Simple illustration of how NewTestSurrealDB can be used.
+
+	// Tests can run in parallel.
+	t.Parallel()
+
+	// NewTestSurrealDB returns endpoint (random open port used), DB instance
+	// (which is wrapped in DBForTest to provide extra methods), and a func to
+	// shutdown the SurrealDB server.
+	endpoint, db, close := NewTestSurrealDB(t)
+	defer close()
+	_ = endpoint // endpoint could be useful for some low level testing.
+
+	// DBForTest.Prepare is to run any SurrealQL for test prep. This would fail
+	// early if the provided string results in an error from SurrealDB server.
+	db.Prepare(t, "CREATE user:x SET name = 'x'")
+
+	// More interactions can simply use db instance from here on.
+}


I added an extra test case to illustrate how the new test util function can be used. Hopefully this would make it a bit clearer how this can be a useful addition.

phughk

Thanks so much @rytswd for this change! I have left quite a few comments, but I think the direction this is going is awesome overall!

Perhaps we could have a test setup like the following

func TestFeature(t *testing.T) {
   // This is how we would set up a database per test
   t.Run("check feature", func(t *testing.T) {
          // the harness module would be used as part of test setup like this, similar to
          // how we use the testify harness
          harness.WithDb(t, func(t *testing.T, db DB) {
               doSomethingThatIsntSharedBetweenParentTests()
          
               // Example of how to share a database between tests
               t.Run("feature property case", func(t *testing.T) {
                   doSomethingThatWillShareStateBetweenTests(db)
               })
               t.Run("another feature property case", func(t *testing.T) {
                  doSomethingThatDoesntConflictWithThePreviousTest(db)
               })
          })
   })
}

phughk · 2023-12-05T09:19:25Z

pkg/conn/gorilla/gorilla.go

@@ -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


Suggested change

// TODO: Handle error correctly, otherwise this can panic

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

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

phughk · 2023-12-05T09:42:18Z

util_for_test.go

+	return db
+}
+
+func (d *DBForTest) Prepare(t testing.TB, schema string) {


Do we want this helper function?

If we use the driver functions directly, as it is intended to be used, then people can refer to it for examples.

This is also code that will need to be maintained. We don't know if the above will result in flakey tests or bugs (it looks good atm, just thinking of future tests).

But you are right that we will often want schemas defined. I wonder if we might want to name this DefineTestSchema instead. That way, it is clear what it does and that it isn't part of the driver.

It is a good point, and I'm absolutely fine dropping this. If we are to drop this, we would want to add simple test cases where we can showcase how the database prep can be done.

Just to give a bit more context and clarity, this code actually comes from my own testing with https://github.com/upsidr/surrealtest, which uses Ory's dockertest package to start a new database. I wanted to set up the tables before going into the main code to be tested, which can fail with t.Fatal, so that I don't get confused about when test error is due to some prep SurrealQL syntax error, rather than code issues.

Side Note: Although not the same syntax, Google Cloud Spanner has spannertest with a dedicated function UpdateDDL https://pkg.go.dev/cloud.google.com/go/spanner/spannertest#Server.UpdateDDL to set up the database. This is because spannertest is only an emulator and not a real database, but this makes it clear that any error from UpdateDDL would be a fatal one and does not have to do with the actual test case.

This is certainly some additional code to be maintained, but test helpers like this take in t.testing.TB, so the usage would be limited. Having testify and using suite is about the same IMHO -- they add some complexity / extra layer for convenience. I believe this is more of a preference, and I'm absolutely happy to go with whichever route.

phughk · 2023-12-05T14:21:32Z

util_for_test.go

+func checkQueryResponse(data interface{}) error {
+	var err error
+	var ok bool
+	d := data


I think you can just reassign data, if you don't need the input later on, no?

data, err = data.([]interface{})

I can't remember exactly how this was needed, but I think the data being interface{} meant that it could be a single result (without any slice involved), but sometimes can be a slice of interface. The code here was meant to facilitate both cases IIRC. If there is a better way than this, I'm absolutely happy to take that -- I would probably need a specific test case why I needed this... (it would take some time for me to dig that out)

phughk · 2023-12-08T08:12:36Z

query_test.go

+		dbInteraction       func(*testing.T, *DB)
+		keepServerUponError bool
+	}{
+		"first case": {


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

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.

phughk · 2023-12-08T08:15:22Z

query_test.go

+				"CREATE user:x SET name = 'xxxx'",
+				"CREATE user:y SET name = 'yyyy'",
+			},
+			dbInteraction: func(t *testing.T, db *DB) {


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.

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.

phughk · 2023-12-08T09:00:57Z

util_for_test.go

+			}
+			// Simply check all fields match as expected.
+			want := dataholder{ID: "user:x", Name: "x", Index: i}
+			if d[0].ID != want.ID || d[0].Name != want.Name || d[0].Index != want.Index {


Using de morgans law, you can translate this from !A || !B into A && B. Since it would become individual cases, you can apply them with an assert without the AND (each case must be true)

Suggested change

if d[0].ID != want.ID || d[0].Name != want.Name || d[0].Index != want.Index {

t.AssertEq(d[0].ID, want.ID)

t.AssertEq(d[0].Name, want.Name)

t.Assert(d[0].Index, want.Index)

This was a really crude check, and only wrote it so that it would be easy to reason about. Usually these checks are written with something like if want != got { /* something like t.Error(err) */ }, and just stuck with the same handling. I'm happy to update this anyhow 👍 (Again, I just chose to go without testify for the test cases in this file.)

One caveat is how the error report shows the want vs d rather than checking specific item such as d[0].Name in my implementation. It is possible that the server returns some extra data (such as d[1], and thus it would be helpful to write everything rather than just asserting on a single field.

phughk · 2023-12-08T09:33:16Z

util_for_test.go

+	// propagate, so that the client can shut down before.
+	// NOTE: There is currently a race condition and potential panic due to the
+	// closed websocket being used in a tight loop.
+	cmd.WaitDelay = DelayBeforeServerExit


I don't fully understand this scenario.

If the db is starting too slowly (i.e. waiting in test before continuing), we can add retries to the driver. Or perhaps try connecting to the port and "unblock" when the port is reachable. When the port is reachable, the db is available.

If the client needs to disconnect before the db, we can do a teardown of the client step. The teardown of the client before teardown of db can be ensured by bundling the two together in a struct and controlling the lifetime of that.

Indeed, this has to do with the websocket connection handling, and this logic should not be necessary. But without this, tests would fail sporadically.

phughk · 2023-12-08T09:35:17Z

util_for_test.go

+	*DB
+}
+
+func NewTestSurrealDB(t testing.TB) (string, *DBForTest, func()) {


I also don't think this needs to be tested separately. I appreciate that the tests were written to have a clear separation between existing tests and added code (added code only adds then, thank you for taking care of that).

I think we would still like to be in a position where the existing code uses this functionality, and the test correctness indicates the correct implementation of the test setup.

I would be happy if you changed some of the tests (or added a new driver-centered test) that uses this setup but instead tests the driver instead of the setup.

I could take a look at the existing test cases as well -- I was just trying to make a small PR and iterate more as we go. As an Open Source project (SDK, tool, or any sort), it would be useful to have more commits to give more clarity. But, as you say, perhaps with test refactoring like this, it may be fine to have a large PR to clean up all of them at once.

phughk · 2023-12-08T09:41:01Z

util_for_test.go

+	if err != nil {
+		t.Fatalf("could not find any open port: %v", err)
+	}
+	l, err := net.ListenTCP("tcp", addr)


If you bind to port 0, the operating system will assign a random port

That's what I do just a few lines above -- but the complication was coming from how I was using net.ListenTCP instead of net.Listen. I have simplified this code to use localhost:0 directly 6afa007

phughk · 2023-12-08T09:55:44Z

util_for_test.go

+				continue
+			}
+
+			// TODO: The server reuses the "result" field for both the actual


We probably want this ticketed. That way we can change the TODO into TODO(gh-123) or whatever the ticket number will be

OK I'll create a separate ticket for this 👍

rytswd · 2023-12-15T17:23:25Z

@phughk Sorry for getting back to you late, I got some quick fixes in, and commented back on some discussion points. It would be great if I can get more of your thoughts! 🥰

rytswd added 2 commits December 1, 2023 22:45

Add flexible test based on surreal cli

78d0e05

Add TODO comment

dca1e95

Add an example query tests based on new util

f9ce745

Ensure to call t.Helper() for helper func

79c32bd

rytswd mentioned this pull request Dec 6, 2023

Bug: Unhandled errors in websockets can lead to panics #119

Closed

2 tasks

rytswd added 2 commits December 6, 2023 12:23

Rename shutdown function to close

fb30b5d

Add simple test case as an example

a52137f

rytswd commented Dec 6, 2023

View reviewed changes

phughk reviewed Dec 8, 2023

View reviewed changes

phughk self-assigned this Dec 8, 2023

rytswd added 2 commits December 15, 2023 15:50

Add issue reference

69ef565

Simplify net.Listen with localhost:0

6afa007

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add flexible test setup #118

Add flexible test setup #118

rytswd commented Dec 1, 2023

ElecTwix commented Dec 3, 2023 •

edited

Loading

rytswd commented Dec 3, 2023

ElecTwix commented Dec 3, 2023

rytswd commented Dec 4, 2023

rytswd Dec 6, 2023

phughk left a comment

phughk Dec 5, 2023

rytswd Dec 15, 2023

phughk Dec 5, 2023

rytswd Dec 15, 2023

phughk Dec 5, 2023

rytswd Dec 15, 2023

phughk Dec 8, 2023

rytswd Dec 15, 2023

phughk Dec 8, 2023

rytswd Dec 15, 2023

phughk Dec 8, 2023

rytswd Dec 15, 2023

phughk Dec 8, 2023

rytswd Dec 15, 2023

phughk Dec 8, 2023

rytswd Dec 15, 2023

phughk Dec 8, 2023

rytswd Dec 15, 2023

phughk Dec 8, 2023

rytswd Dec 15, 2023

rytswd commented Dec 15, 2023

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

-			if d[0].ID != want.ID || d[0].Name != want.Name || d[0].Index != want.Index {
+			t.AssertEq(d[0].ID, want.ID)
+			t.AssertEq(d[0].Name, want.Name)
+			t.Assert(d[0].Index, want.Index)

Add flexible test setup #118

Are you sure you want to change the base?

Add flexible test setup #118

Conversation

rytswd commented Dec 1, 2023

WHAT

WHY

HOW

Other Notes

ElecTwix commented Dec 3, 2023 • edited Loading

rytswd commented Dec 3, 2023

ElecTwix commented Dec 3, 2023

Conclusion

About Gorilla

rytswd commented Dec 4, 2023

Choose a reason for hiding this comment

phughk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rytswd commented Dec 15, 2023

ElecTwix commented Dec 3, 2023 •

edited

Loading