You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@kvrocks.apache.org by GitBox <gi...@apache.org> on 2022/09/01 18:19:25 UTC
[GitHub] [incubator-kvrocks] tisonkun opened a new pull request, #806: refactor: write integration tests in Golang
tisonkun opened a new pull request, #806:
URL: https://github.com/apache/incubator-kvrocks/pull/806
## Motivation
TCL scripts are hard for debugging. It helps to bring Redis tests but we can later refactor them for easy maintaining and debugging.
I ever try to write it in Rust https://github.com/apache/incubator-kvrocks/discussions/693 but finally propose this patch in Golang. As Rust's testing is less than mature and our contributors like @git-hulk & @torwig should be familiar with Golang. Also, go-redis project has [a backlink to Kvrocks](https://github.com/go-redis/redis#ecosystem) and when possibly, I'm glad to use it for Kvrocks' tests.
## Implementation
1. Add go integration cases mod under `tests/gocase`.
2. Implement the start server logic at `util/server.go`
3. Migrate TCL test `util/command` to the new integration framework.
4. Wire logic to the CI workflow.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on pull request #806: refactor: write integration tests in Golang
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #806:
URL: https://github.com/apache/incubator-kvrocks/pull/806#issuecomment-1236059589
I looked through this PR and it’s clear enough for me. To see other guys have any questions? @PragmaTwice @ShooterIT @Alfejik @caipengbo
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #806: refactor: write integration tests in Golang
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #806:
URL: https://github.com/apache/incubator-kvrocks/pull/806#discussion_r961370964
##########
.github/workflows/kvrocks.yaml:
##########
@@ -160,6 +160,27 @@ jobs:
export ${{ matrix.runtime_env_vars }}
./build/unittest
+ - name: Setup Go
+ uses: actions/setup-go@v3
+ with:
+ go-version: 1.19.x
+
+ - name: Run Go Integration Cases
+ working-directory: tests/gocase
+ run: |
+ export ${{ matrix.runtime_env_vars }}
+ export KVROCKS_BIN_PATH="$GITHUB_WORKSPACE/build/kvrocks"
+ export GO_CASE_WORKSPACE="$GITHUB_WORKSPACE/tests/gocase/workspace"
+ go test -v ./...
Review Comment:
Could these be added in `./x.py`, like `./x.py test go <build-dir>`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] tisonkun merged pull request #806: refactor: write integration tests in Golang
Posted by GitBox <gi...@apache.org>.
tisonkun merged PR #806:
URL: https://github.com/apache/incubator-kvrocks/pull/806
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #806: refactor: write integration tests in Golang
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #806:
URL: https://github.com/apache/incubator-kvrocks/pull/806#discussion_r961409632
##########
tests/gocase/util/server.go:
##########
@@ -0,0 +1,119 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+package util
+
+import (
+ "context"
+ "fmt"
+ "github.com/go-redis/redis/v9"
+ "github.com/stretchr/testify/require"
+ "net"
+ "os"
+ "os/exec"
+ "testing"
+ "time"
+)
+
+type KvrocksServer struct {
+ t *testing.T
+ cmd *exec.Cmd
+ r *redis.Client
+
+ clean func()
+}
+
+func (s *KvrocksServer) Client() *redis.Client {
+ return s.r
+}
+
+func (s *KvrocksServer) Close() {
+ require.NoError(s.t, s.cmd.Process.Kill())
+ require.EqualError(s.t, s.cmd.Wait(), "signal: killed")
+ s.clean()
+}
+
+func StartServer(t *testing.T, configs map[string]string) (*KvrocksServer, error) {
+ b := os.Getenv("KVROCKS_BIN_PATH")
+ cmd := exec.Command(b)
+
+ addr, err := findFreePort()
+ if err != nil {
+ return nil, err
+ }
+ configs["bind"] = addr.IP.String()
+ configs["port"] = fmt.Sprintf("%d", addr.Port)
+
+ dir := os.Getenv("GO_CASE_WORKSPACE")
+ require.NoError(t, err)
+ dir, err = os.MkdirTemp(dir, "Server-*")
+ require.NoError(t, err)
+ configs["dir"] = dir
+
+ f, err := os.CreateTemp(dir, "*.conf")
Review Comment:
I think `kvrocks.conf` is enough rather than a random string? We can locate it more easy if it has a fixed name.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #806: refactor: write integration tests in Golang
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #806:
URL: https://github.com/apache/incubator-kvrocks/pull/806#discussion_r961407066
##########
tests/tcl/tests/test_helper.tcl:
##########
@@ -56,7 +56,6 @@ set ::all_tests {
unit/introspection
unit/limits
unit/geo
- unit/command
Review Comment:
Sounds good to me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] tisonkun commented on a diff in pull request #806: refactor: write integration tests in Golang
Posted by GitBox <gi...@apache.org>.
tisonkun commented on code in PR #806:
URL: https://github.com/apache/incubator-kvrocks/pull/806#discussion_r961492184
##########
tests/gocase/util/server.go:
##########
@@ -0,0 +1,119 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+package util
+
+import (
+ "context"
+ "fmt"
+ "github.com/go-redis/redis/v9"
+ "github.com/stretchr/testify/require"
+ "net"
+ "os"
+ "os/exec"
+ "testing"
+ "time"
+)
+
+type KvrocksServer struct {
+ t *testing.T
+ cmd *exec.Cmd
+ r *redis.Client
+
+ clean func()
+}
+
+func (s *KvrocksServer) Client() *redis.Client {
+ return s.r
+}
+
+func (s *KvrocksServer) Close() {
+ require.NoError(s.t, s.cmd.Process.Kill())
+ require.EqualError(s.t, s.cmd.Wait(), "signal: killed")
+ s.clean()
+}
+
+func StartServer(t *testing.T, configs map[string]string) (*KvrocksServer, error) {
+ b := os.Getenv("KVROCKS_BIN_PATH")
+ cmd := exec.Command(b)
+
+ addr, err := findFreePort()
+ if err != nil {
+ return nil, err
+ }
+ configs["bind"] = addr.IP.String()
+ configs["port"] = fmt.Sprintf("%d", addr.Port)
+
+ dir := os.Getenv("GO_CASE_WORKSPACE")
+ require.NoError(t, err)
+ dir, err = os.MkdirTemp(dir, "Server-*")
+ require.NoError(t, err)
+ configs["dir"] = dir
+
+ f, err := os.CreateTemp(dir, "*.conf")
Review Comment:
Updated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #806: refactor: write integration tests in Golang
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #806:
URL: https://github.com/apache/incubator-kvrocks/pull/806#discussion_r961398061
##########
tests/tcl/tests/test_helper.tcl:
##########
@@ -56,7 +56,6 @@ set ::all_tests {
unit/introspection
unit/limits
unit/geo
- unit/command
Review Comment:
If we delete it in the migration procedure, developers may need a two phase testing in their local:
1. run tcl tests
2. run go tests
Any idea to simplify the test executing?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] tisonkun commented on a diff in pull request #806: refactor: write integration tests in Golang
Posted by GitBox <gi...@apache.org>.
tisonkun commented on code in PR #806:
URL: https://github.com/apache/incubator-kvrocks/pull/806#discussion_r961420594
##########
tests/gocase/util/server.go:
##########
@@ -0,0 +1,119 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+package util
+
+import (
+ "context"
+ "fmt"
+ "github.com/go-redis/redis/v9"
+ "github.com/stretchr/testify/require"
+ "net"
+ "os"
+ "os/exec"
+ "testing"
+ "time"
+)
+
+type KvrocksServer struct {
+ t *testing.T
+ cmd *exec.Cmd
+ r *redis.Client
+
+ clean func()
+}
+
+func (s *KvrocksServer) Client() *redis.Client {
+ return s.r
+}
+
+func (s *KvrocksServer) Close() {
+ require.NoError(s.t, s.cmd.Process.Kill())
+ require.EqualError(s.t, s.cmd.Wait(), "signal: killed")
+ s.clean()
+}
+
+func StartServer(t *testing.T, configs map[string]string) (*KvrocksServer, error) {
+ b := os.Getenv("KVROCKS_BIN_PATH")
+ cmd := exec.Command(b)
+
+ addr, err := findFreePort()
+ if err != nil {
+ return nil, err
+ }
+ configs["bind"] = addr.IP.String()
+ configs["port"] = fmt.Sprintf("%d", addr.Port)
+
+ dir := os.Getenv("GO_CASE_WORKSPACE")
+ require.NoError(t, err)
+ dir, err = os.MkdirTemp(dir, "Server-*")
+ require.NoError(t, err)
+ configs["dir"] = dir
+
+ f, err := os.CreateTemp(dir, "*.conf")
Review Comment:
Good point! Will push a commit for it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] tisonkun commented on pull request #806: refactor: write integration tests in Golang
Posted by GitBox <gi...@apache.org>.
tisonkun commented on PR #806:
URL: https://github.com/apache/incubator-kvrocks/pull/806#issuecomment-1236116253
It happens that I have some time to work on this migration. I'll merge this patch tomorrow if no more objections and continue the tasks.
Feel free to review after merge.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on pull request #806: refactor: write integration tests in Golang
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #806:
URL: https://github.com/apache/incubator-kvrocks/pull/806#issuecomment-1234997555
Thanks @tisonkun
The tcl is really hard to debug for myself. Go or Rust are good to me but I think Go will be easier for developers to learn and participate. Can see other guys have any feedback on this topic,
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] tisonkun commented on a diff in pull request #806: refactor: write integration tests in Golang
Posted by GitBox <gi...@apache.org>.
tisonkun commented on code in PR #806:
URL: https://github.com/apache/incubator-kvrocks/pull/806#discussion_r961386880
##########
.github/workflows/kvrocks.yaml:
##########
@@ -160,6 +160,27 @@ jobs:
export ${{ matrix.runtime_env_vars }}
./build/unittest
+ - name: Setup Go
+ uses: actions/setup-go@v3
+ with:
+ go-version: 1.19.x
+
+ - name: Run Go Integration Cases
+ working-directory: tests/gocase
+ run: |
+ export ${{ matrix.runtime_env_vars }}
+ export KVROCKS_BIN_PATH="$GITHUB_WORKSPACE/build/kvrocks"
+ export GO_CASE_WORKSPACE="$GITHUB_WORKSPACE/tests/gocase/workspace"
+ go test -v ./...
Review Comment:
Yes, it's possible. I'd like to do it as a follow-up, though.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] tisonkun commented on a diff in pull request #806: refactor: write integration tests in Golang
Posted by GitBox <gi...@apache.org>.
tisonkun commented on code in PR #806:
URL: https://github.com/apache/incubator-kvrocks/pull/806#discussion_r961400415
##########
tests/tcl/tests/test_helper.tcl:
##########
@@ -56,7 +56,6 @@ set ::all_tests {
unit/introspection
unit/limits
unit/geo
- unit/command
Review Comment:
If we don't delete it, the situation doesn't change. If only the migration can be eventually closed, I don't think it's a big deal.
Since we do have TCL tests and go IT cases, we always run both. There's no way to avoid it. Only you may batch them in one script call but it seems not necessary.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #806: refactor: write integration tests in Golang
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #806:
URL: https://github.com/apache/incubator-kvrocks/pull/806#discussion_r961393295
##########
.github/workflows/kvrocks.yaml:
##########
@@ -160,6 +160,27 @@ jobs:
export ${{ matrix.runtime_env_vars }}
./build/unittest
+ - name: Setup Go
+ uses: actions/setup-go@v3
+ with:
+ go-version: 1.19.x
+
+ - name: Run Go Integration Cases
+ working-directory: tests/gocase
+ run: |
+ export ${{ matrix.runtime_env_vars }}
+ export KVROCKS_BIN_PATH="$GITHUB_WORKSPACE/build/kvrocks"
+ export GO_CASE_WORKSPACE="$GITHUB_WORKSPACE/tests/gocase/workspace"
+ go test -v ./...
Review Comment:
got it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org