Go: test fixes + reporting (valkey-io#2867)

* test fixes + reporting

Signed-off-by: Yury-Fridlyand <[email protected]>

.github/workflows/go.yml

@@ -102,8 +102,8 @@ jobs:
             - name: Install & build & test
               working-directory: go
               run: |
-                  LD_LIBRARY_PATH=$LD_LIBRARY_PATH:$GITHUB_WORKSPACE/go/target/release/deps/
-                  make install-tools-go${{ matrix.go }} build unit-test integ-test
+                  make install-tools-go${{ matrix.go }} build
+                  make -k unit-test integ-test
 
             - uses: ./.github/workflows/test-benchmark
               with:
@@ -118,6 +118,7 @@ jobs:
                   path: |
                       utils/clusters/**
                       benchmarks/results/**
+                      go/reports/**
 
     lint:
         timeout-minutes: 10
@@ -205,8 +206,8 @@ jobs:
             - name: Install & build & test
               working-directory: go
               run: |
-                  LD_LIBRARY_PATH=$LD_LIBRARY_PATH:$GITHUB_WORKSPACE/go/target/release/deps/
-                  make install-tools-go${{ matrix.go }} build unit-test integ-test
+                  make install-tools-go${{ matrix.go }} build
+                  make -k unit-test integ-test
 
             - name: Upload test reports
               if: always()
@@ -217,6 +218,7 @@ jobs:
                   path: |
                       utils/clusters/**
                       benchmarks/results/**
+                      go/reports/**
 
     test-modules:
         if: (github.repository_owner == 'valkey-io' && github.event_name == 'workflow_dispatch') || github.event.pull_request.head.repo.owner.login == 'valkey-io'
@@ -239,10 +241,13 @@ jobs:
             - name: Build and test
               working-directory: ./go
               run: |
-                  make install-tools-go1.20.0
-                  LD_LIBRARY_PATH=$LD_LIBRARY_PATH:$GITHUB_WORKSPACE/go/target/release/deps/
-                  make build
-                  make modules-test cluster-endpoints=${{ secrets.MEMDB_MODULES_ENDPOINT }} tls=true
+                  make install-tools-go1.20.0 build modules-test cluster-endpoints=${{ secrets.MEMDB_MODULES_ENDPOINT }} tls=true
 
-            # TODO:
-            # Upload test reports
+            - name: Upload test reports
+              if: always()
+              continue-on-error: true
+              uses: actions/upload-artifact@v4
+              with:
+                  name: test-reports-modules
+                  path: |
+                      go/reports/**

go/DEVELOPER.md

@@ -167,6 +167,8 @@ By default, those test suite start standalone and cluster servers without TLS an
 make integ-test standalone-endpoints=localhost:6379 cluster-endpoints=localhost:7000 tls=true
 ```
 
+Test reports generated in `reports` folder.
+
 ### Generate protobuf files
 
 During the initial build, Go protobuf files were created in `go/protobuf`. If modifications are made to the protobuf definition files (.proto files located in `glide-core/src/protobuf`), it becomes necessary to regenerate the Go protobuf files. To do so, run:

go/Makefile

@@ -1,3 +1,5 @@
+SHELL:=/bin/bash
+
 install-build-tools:
 	go install google.golang.org/protobuf/cmd/[email protected]
 	cargo install cbindgen
@@ -40,6 +42,7 @@ clean:
 	rm -f benchmarks/benchmarks
 	rm -rf protobuf
 	rm -rf target
+	rm -rf reports
 
 build-glide-client:
 	cargo build --release
@@ -76,8 +79,11 @@ format:
 
 # unit tests - skip complete IT suite (including MT)
 unit-test:
+	mkdir -p reports
+	set -o pipefail; \
 	LD_LIBRARY_PATH=$(shell find . -name libglide_rs.so|grep -w release|tail -1|xargs dirname|xargs readlink -f):${LD_LIBRARY_PATH} \
-	go test -v -race ./... -skip TestGlideTestSuite $(if $(test-filter), -run $(test-filter))
+	go test -v -race ./... -skip TestGlideTestSuite $(if $(test-filter), -run $(test-filter)) \
+	| tee >(go tool test2json -t -p github.com/valkey-io/valkey-glide/go/glide/utils | go-test-report -o reports/unit-tests.html -t unit-test > /dev/null)
 
 # integration tests - run subtask with skipping modules tests
 integ-test: export TEST_FILTER = -skip TestGlideTestSuite/TestModule $(if $(test-filter), -run $(test-filter))
@@ -88,17 +94,14 @@ modules-test: export TEST_FILTER = $(if $(test-filter), -run $(test-filter), -ru
 modules-test: __it
 
 __it:
+	mkdir -p reports
+	set -o pipefail; \
 	LD_LIBRARY_PATH=$(shell find . -name libglide_rs.so|grep -w release|tail -1|xargs dirname|xargs readlink -f):${LD_LIBRARY_PATH} \
 	go test -v -race ./integTest/... \
-		$(TEST_FILTER) \
-		$(if $(filter true, $(tls)), --tls,) \
-		$(if $(standalone-endpoints), --standalone-endpoints=$(standalone-endpoints)) \
-		$(if $(cluster-endpoints), --cluster-endpoints=$(cluster-endpoints))
-
-# Note: this task is no longer run by CI because:
-# - build failures that occur while running the task can be hidden by the task; CI still reports success in these scenarios.
-# - there is not a good way to both generate a test report and log the test outcomes to GH actions.
-# TODO: fix this and include -run/-skip flags
-test-and-report:
-	mkdir -p reports
-	go test -v -race ./... -json | go-test-report -o reports/test-report.html
+	$(TEST_FILTER) \
+	$(if $(filter true, $(tls)), --tls,) \
+	$(if $(standalone-endpoints), --standalone-endpoints=$(standalone-endpoints)) \
+	$(if $(cluster-endpoints), --cluster-endpoints=$(cluster-endpoints)) \
+	| tee >(go tool test2json -t -p github.com/valkey-io/valkey-glide/go/glide/integTest | go-test-report -o reports/integ-tests.html -t integ-test > /dev/null)
+# code above ^ is similar to `go test .... -json | go-test-report ....`, but it also prints plain text output to stdout
+# `go test` prints plain text, tee duplicates it to stdout and to `test2json` which is coupled with `go-test-report` to generate the report

go/api/options/zadd_options.go

@@ -22,8 +22,7 @@ func NewZAddOptionsBuilder() *ZAddOptions {
 	return &ZAddOptions{}
 }
 
-// `conditionalChange“ defines conditions for updating or adding elements with {@link SortedSetBaseCommands#zadd}
-// command.
+// `conditionalChange` defines conditions for updating or adding elements with `ZADD` command.
 func (options *ZAddOptions) SetConditionalChange(c ConditionalChange) *ZAddOptions {
 	options.conditionalChange = c
 	return options

go/integTest/glide_test_suite_test.go

@@ -264,8 +264,8 @@ func (suite *GlideTestSuite) clusterClient(config *api.GlideClusterClientConfigu
 }
 
 func (suite *GlideTestSuite) runWithClients(clients []api.BaseClient, test func(client api.BaseClient)) {
-	for i, client := range clients {
-		suite.T().Run(fmt.Sprintf("Testing [%v]", i), func(t *testing.T) {
+	for _, client := range clients {
+		suite.T().Run(fmt.Sprintf("%T", client)[5:], func(t *testing.T) {
 			test(client)
 		})
 	}

go/integTest/shared_commands_test.go

@@ -55,7 +55,7 @@ func (suite *GlideTestSuite) TestSetWithOptions_ReturnOldValue() {
 
 func (suite *GlideTestSuite) TestSetWithOptions_OnlyIfExists_overwrite() {
 	suite.runWithDefaultClients(func(client api.BaseClient) {
-		key := "TestSetWithOptions_OnlyIfExists_overwrite"
+		key := uuid.New().String()
 		suite.verifyOK(client.Set(key, initialValue))
 
 		opts := api.NewSetOptionsBuilder().SetConditionalSet(api.OnlyIfExists)
@@ -70,7 +70,7 @@ func (suite *GlideTestSuite) TestSetWithOptions_OnlyIfExists_overwrite() {
 
 func (suite *GlideTestSuite) TestSetWithOptions_OnlyIfExists_missingKey() {
 	suite.runWithDefaultClients(func(client api.BaseClient) {
-		key := "TestSetWithOptions_OnlyIfExists_missingKey"
+		key := uuid.New().String()
 		opts := api.NewSetOptionsBuilder().SetConditionalSet(api.OnlyIfExists)
 		result, err := client.SetWithOptions(key, anotherValue, opts)
 
@@ -81,7 +81,7 @@ func (suite *GlideTestSuite) TestSetWithOptions_OnlyIfExists_missingKey() {
 
 func (suite *GlideTestSuite) TestSetWithOptions_OnlyIfDoesNotExist_missingKey() {
 	suite.runWithDefaultClients(func(client api.BaseClient) {
-		key := "TestSetWithOptions_OnlyIfDoesNotExist_missingKey"
+		key := uuid.New().String()
 		opts := api.NewSetOptionsBuilder().SetConditionalSet(api.OnlyIfDoesNotExist)
 		suite.verifyOK(client.SetWithOptions(key, anotherValue, opts))
 
@@ -94,7 +94,7 @@ func (suite *GlideTestSuite) TestSetWithOptions_OnlyIfDoesNotExist_missingKey()
 
 func (suite *GlideTestSuite) TestSetWithOptions_OnlyIfDoesNotExist_existingKey() {
 	suite.runWithDefaultClients(func(client api.BaseClient) {
-		key := "TestSetWithOptions_OnlyIfDoesNotExist_existingKey"
+		key := uuid.New().String()
 		opts := api.NewSetOptionsBuilder().SetConditionalSet(api.OnlyIfDoesNotExist)
 		suite.verifyOK(client.Set(key, initialValue))
 
@@ -112,7 +112,7 @@ func (suite *GlideTestSuite) TestSetWithOptions_OnlyIfDoesNotExist_existingKey()
 
 func (suite *GlideTestSuite) TestSetWithOptions_KeepExistingExpiry() {
 	suite.runWithDefaultClients(func(client api.BaseClient) {
-		key := "TestSetWithOptions_KeepExistingExpiry"
+		key := uuid.New().String()
 		opts := api.NewSetOptionsBuilder().SetExpiry(api.NewExpiryBuilder().SetType(api.Milliseconds).SetCount(uint64(2000)))
 		suite.verifyOK(client.SetWithOptions(key, initialValue, opts))
 
@@ -139,7 +139,7 @@ func (suite *GlideTestSuite) TestSetWithOptions_KeepExistingExpiry() {
 
 func (suite *GlideTestSuite) TestSetWithOptions_UpdateExistingExpiry() {
 	suite.runWithDefaultClients(func(client api.BaseClient) {
-		key := "TestSetWithOptions_UpdateExistingExpiry"
+		key := uuid.New().String()
 		opts := api.NewSetOptionsBuilder().SetExpiry(api.NewExpiryBuilder().SetType(api.Milliseconds).SetCount(uint64(100500)))
 		suite.verifyOK(client.SetWithOptions(key, initialValue, opts))
 
@@ -166,14 +166,14 @@ func (suite *GlideTestSuite) TestSetWithOptions_UpdateExistingExpiry() {
 
 func (suite *GlideTestSuite) TestGetEx_existingAndNonExistingKeys() {
 	suite.runWithDefaultClients(func(client api.BaseClient) {
-		key := "TestGetEx_ExisitingKey"
+		key := uuid.New().String()
 		suite.verifyOK(client.Set(key, initialValue))
 
 		result, err := client.GetEx(key)
 		assert.Nil(suite.T(), err)
 		assert.Equal(suite.T(), initialValue, result.Value())
 
-		key = "TestGetEx_NonExisitingKey"
+		key = uuid.New().String()
 		result, err = client.Get(key)
 		assert.Nil(suite.T(), err)
 		assert.Equal(suite.T(), "", result.Value())
@@ -182,7 +182,7 @@ func (suite *GlideTestSuite) TestGetEx_existingAndNonExistingKeys() {
 
 func (suite *GlideTestSuite) TestGetExWithOptions_PersistKey() {
 	suite.runWithDefaultClients(func(client api.BaseClient) {
-		key := "TestGetExWithOptions_PersistKey"
+		key := uuid.New().String()
 		suite.verifyOK(client.Set(key, initialValue))
 
 		opts := api.NewGetExOptionsBuilder().SetExpiry(api.NewExpiryBuilder().SetType(api.Milliseconds).SetCount(uint64(2000)))
@@ -205,7 +205,7 @@ func (suite *GlideTestSuite) TestGetExWithOptions_PersistKey() {
 
 func (suite *GlideTestSuite) TestGetExWithOptions_UpdateExpiry() {
 	suite.runWithDefaultClients(func(client api.BaseClient) {
-		key := "TestGetExWithOptions_UpdateExpiry"
+		key := uuid.New().String()
 		suite.verifyOK(client.Set(key, initialValue))
 
 		opts := api.NewGetExOptionsBuilder().SetExpiry(api.NewExpiryBuilder().SetType(api.Milliseconds).SetCount(uint64(2000)))
@@ -227,7 +227,7 @@ func (suite *GlideTestSuite) TestGetExWithOptions_UpdateExpiry() {
 
 func (suite *GlideTestSuite) TestSetWithOptions_ReturnOldValue_nonExistentKey() {
 	suite.runWithDefaultClients(func(client api.BaseClient) {
-		key := "TestSetWithOptions_ReturnOldValue_nonExistentKey"
+		key := uuid.New().String()
 		opts := api.NewSetOptionsBuilder().SetReturnOldValue(true)
 
 		result, err := client.SetWithOptions(key, anotherValue, opts)