Add flexible test setup

[rytswd]

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]

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]

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]

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: https://github.com/gorilla/websocket/issues/657 https://github.com/gorilla/websocket/issues/474

[rytswd]

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:

1. With the current test setup (regardless of testify used or not), it assumes there is SurrealDB server running at localhost:8000
2. With the current test setup (regardless of testify used or not), we cannot run tests in parallel because it uses a single instance
3. 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
4. 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]

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