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.

Sign up for GitHub

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 go integrate test #1792

Merged
merged 26 commits into from
Aug 3, 2023
Merged

add go integrate test #1792

Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
37558c4
add go integrate test
luky116 Jul 22, 2023
eff05af
add go integrate test
luky116 Jul 22, 2023
0f5351c
add go integrate test
luky116 Jul 22, 2023
0013131
add go integrate test
luky116 Jul 22, 2023
958730b
temp test
luky116 Jul 22, 2023
c3a25f4
temp test
luky116 Jul 22, 2023
fc0d3dd
temp test
luky116 Jul 22, 2023
02be838
temp test
luky116 Jul 22, 2023
444e6d3
temp test
luky116 Jul 22, 2023
cb8bda1
temp test
luky116 Jul 22, 2023
301e599
add test
luky116 Jul 22, 2023
780da7f
temp test
luky116 Jul 22, 2023
ad89658
temp test
luky116 Jul 22, 2023
de90d53
add tcl
luky116 Jul 22, 2023
da62b85
Merge remote-tracking branch 'pika/unstable' into addGoIntegrateTest
luky116 Jul 22, 2023
859436b
add tcl
luky116 Jul 22, 2023
663c12c
reformat code
luky116 Jul 22, 2023
cdd09f8
add tcl
luky116 Jul 22, 2023
e4a8048
reformat code
luky116 Jul 23, 2023
86501ad
add tcl
luky116 Jul 23, 2023
13755aa
reformat code
luky116 Jul 23, 2023
db1428f
add tcl
luky116 Jul 23, 2023
c1f6c83
fix setxx
luky116 Jul 23, 2023
bc91da9
fix setxx
luky116 Jul 23, 2023
d50a0f5
add tcl
luky116 Jul 23, 2023
4f83245
add tcl
luky116 Jul 23, 2023
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
33 changes: 32 additions & 1 deletion .github/workflows/pika.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,11 @@ jobs:
steps:
- uses: actions/checkout@v3

- name: Set up Go
uses: actions/setup-go@v3
with:
go-version: 1.19

- name: cache dependencies
uses: actions/cache@v2
id: cache
Expand Down Expand Up @@ -69,6 +74,13 @@ jobs:
python3 ../tests/integration/pika_replication_test.py
python3 ../tests/unit/Blpop_Brpop_test.py

- name: Run Go E2E Tests
working-directory: ${{ github.workspace }}/build
run: |
cd ../tests/integration/
chmod +x integrate_test.sh
sh integrate_test.sh

build_on_centos:
runs-on: ubuntu-latest
container:
Expand Down Expand Up @@ -123,12 +135,24 @@ jobs:
python3 ../tests/integration/pika_replication_test.py
python3 ../tests/unit/Blpop_Brpop_test.py

- name: Run Go E2E Tests
working-directory: ${{ github.workspace }}/build
run: |
cd ../tests/integration/
chmod +x integrate_test.sh
sh integrate_test.sh

build_on_macos:
runs-on: macos-latest

steps:
- uses: actions/checkout@v3

- name: Set up Go
uses: actions/setup-go@v3
with:
go-version: 1.19

- name: cache dependencies
uses: actions/cache@v2
id: cache
Expand Down Expand Up @@ -175,4 +199,11 @@ jobs:
working-directory: ${{ github.workspace }}/build
run: |
python3 ../tests/integration/pika_replication_test.py
python3 ../tests/unit/Blpop_Brpop_test.py
python3 ../tests/unit/Blpop_Brpop_test.py

- name: Run Go E2E Tests
working-directory: ${{ github.workspace }}/build
run: |
cd ../tests/integration/
chmod +x integrate_test.sh
sh integrate_test.sh
Copy link

Choose a reason for hiding this comment

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

Here are some observations and suggestions for the code patch:

  1. In all three jobs (build_on_ubuntu, build_on_centos, build_on_macos), there is a duplication of steps related to running Go E2E tests. It could be abstracted into a reusable template to avoid repetition.

  2. The file paths used in script execution (python3 ../tests/integration/pika_replication_test.py and python3 ../tests/unit/Blpop_Brpop_test.py) should be validated to ensure they exist in the expected locations.

  3. It's recommended to specify the exact version of actions being used (e.g., actions/checkout@v3) to keep the behavior consistent, rather than relying on the default version.

  4. Consider providing more information or details in the comments about what each job and step does, especially if it's not immediately clear from the context.

  5. Overall, the code patch appears to be fine as long as the referenced files and directories exist and are correctly placed.

Note: It's helpful to provide more details or specific areas you want to address for a more comprehensive review.

Copy link

Choose a reason for hiding this comment

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

Code Review:

  • The code appears to set up Go using the actions/setup-go action in each of the jobs (Set up Go step). This ensures that the required Go version (1.19) is installed before running the tests.
  • In the cache dependencies step, the actions/cache action is used to cache dependencies, which can improve the speed of subsequent workflow runs by reusing the cached dependencies.
  • There are multiple steps to run Python tests in each job, one for pika replication test and another for Blpop_Brpop test. These steps seem fine, assuming the test scripts are correctly implemented and located at the specified paths.
  • Additionally, there is a separate step to run Go E2E tests in each job. The integrate_test.sh script is being invoked in each case.
  • Lastly, the code uses the actions/checkout action to checkout the repository in each job, which allows the workflow to access the repository's files.

Improvement Suggestions:

  • It seems that the steps to set up Go and run Go E2E tests (Run Go E2E Tests) are duplicated in each job. You can consider moving these steps to a separate reusable job template or creating a reusable GitHub Action, so that they don't need to be repeated multiple times.
  • Consider adding more error handling and validation within the script files (integrate_test.sh, test scripts) to handle any failures or unexpected scenarios gracefully.
  • Ensure that the required versions of Go, Python, and other dependencies are accurately specified and compatible with the tests and project requirements.
  • It may be helpful to include additional logging and output statements within the workflow to provide better visibility and understanding of the execution process.

Bug Risk:
Without having access to the content of the integrate_test.sh script and the actual test scripts, it's challenging to determine if there are any specific bug risks. It's important to thoroughly review and test the scripts to ensure their correctness and reliability.

Overall, the code seems to follow a reasonable structure for running tests using different job configurations. However, a more detailed review of the actual test implementations would be necessary to identify any potential issues or improvements specific to the tests themselves.

Copy link

Choose a reason for hiding this comment

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

Overall, the code patch appears to add Go E2E tests and make use of the actions/setup-go action to set up the Go environment. Here are some improvements and potential bug risks that you may want to consider:

  1. Specify a stable Go version: It's generally recommended to specify a stable Go version rather than just 1.19, as versions can change over time. Consider using a specific version that is known to work well with your code.

  2. Clarify the purpose of the cache dependencies step: In the provided code patch, there is a step named "cache dependencies" that uses actions/cache. It's not clear what exactly is being cached and why. Make sure you're caching necessary dependencies to improve build times, but avoid caching directories or files that change frequently.

  3. Review integration test script: Take a closer look at the integrate_test.sh script that is being invoked in both the build_on_centos job and one of the existing jobs. Verify that the script handles errors properly and provides meaningful output.

  4. Maintain consistency across jobs: Ensure that similar steps, such as setting up Go, are consistent across different jobs if they serve the same purpose. In the given code patch, the Set up Go step is duplicated in multiple jobs. Consider factoring out common steps into reusable actions to avoid code duplication and simplify maintenance.

  5. Check for proper error handling and logging: Ensure that all the scripts and commands being executed have proper error handling and logging mechanisms. This will help detect and troubleshoot issues more effectively during the CI/CD process.

  6. Test and verify the overall pipeline flow: Run the modified pipeline and verify that the new Go E2E tests run successfully and produce the expected results. Also, ensure that any existing functionality is not disrupted by the changes introduced.

Remember to thoroughly test and validate the modified pipeline in a staging or non-production environment before deploying it to production.

1 change: 1 addition & 0 deletions src/pika_list.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -331,6 +331,7 @@ void LPopCmd::DoInitial() {
void LPopCmd::Do(std::shared_ptr<Slot> slot) {
std::vector<std::string> elements;
rocksdb::Status s = slot->db()->LPop(key_, count_, &elements);

if (s.ok()) {
res_.AppendArrayLenUint64(elements.size());
for (const auto& element : elements) {
Expand Down
89 changes: 89 additions & 0 deletions tests/integration/csanning_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
package pika_integration

import (
"context"
"fmt"

. "github.com/bsm/ginkgo/v2"
. "github.com/bsm/gomega"
"github.com/redis/go-redis/v9"
)

var _ = Describe("Csanning Commands", func() {
ctx := context.TODO()
var client *redis.Client

BeforeEach(func() {
client = redis.NewClient(pikarOptions1())
Expect(client.FlushDB(ctx).Err()).NotTo(HaveOccurred())
})

AfterEach(func() {
Expect(client.Close()).NotTo(HaveOccurred())
})

Describe("scanning", func() {
It("should Scan", func() {
for i := 0; i < 1000; i++ {
set := client.Set(ctx, fmt.Sprintf("key%d", i), "hello", 0)
Expect(set.Err()).NotTo(HaveOccurred())
}

keys, cursor, err := client.Scan(ctx, 0, "", 0).Result()
Expect(err).NotTo(HaveOccurred())
Expect(keys).NotTo(BeEmpty())
Expect(cursor).NotTo(BeZero())
})

It("should ScanType", func() {
for i := 0; i < 1000; i++ {
set := client.Set(ctx, fmt.Sprintf("key%d", i), "hello", 0)
Expect(set.Err()).NotTo(HaveOccurred())
}

keys, cursor, err := client.ScanType(ctx, 0, "", 0, "").Result()
Expect(err).NotTo(HaveOccurred())
Expect(keys).NotTo(BeEmpty())
Expect(cursor).NotTo(BeZero())
})

It("should SScan", func() {
for i := 0; i < 1000; i++ {
sadd := client.SAdd(ctx, "myset", fmt.Sprintf("member%d", i))
Expect(sadd.Err()).NotTo(HaveOccurred())
}

keys, cursor, err := client.SScan(ctx, "myset", 0, "", 0).Result()
Expect(err).NotTo(HaveOccurred())
Expect(keys).NotTo(BeEmpty())
Expect(cursor).NotTo(BeZero())
})

It("should HScan", func() {
for i := 0; i < 1000; i++ {
sadd := client.HSet(ctx, "myhash", fmt.Sprintf("key%d", i), "hello")
Expect(sadd.Err()).NotTo(HaveOccurred())
}

keys, cursor, err := client.HScan(ctx, "myhash", 0, "", 0).Result()
Expect(err).NotTo(HaveOccurred())
Expect(keys).NotTo(BeEmpty())
Expect(cursor).NotTo(BeZero())
})

It("should ZScan", func() {
for i := 0; i < 1000; i++ {
err := client.ZAdd(ctx, "myset", redis.Z{
Score: float64(i),
Member: fmt.Sprintf("member%d", i),
}).Err()
Expect(err).NotTo(HaveOccurred())
}

keys, cursor, err := client.ZScan(ctx, "myset", 0, "", 0).Result()
Expect(err).NotTo(HaveOccurred())
Expect(keys).NotTo(BeEmpty())
Expect(cursor).NotTo(BeZero())
})
})
})
Copy link

Choose a reason for hiding this comment

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

Overall, the code patch appears to be a test suite for scanning commands in a Redis client. Here are some observations and suggestions for improvement:

  1. Package Name and Organization: Ensure that the package name pika_integration aligns with the directory structure where this code resides. Verify if this is intentional or needs to be updated.

  2. Imports:

  • The use of relative imports (. "github.com/bsm/ginkgo/v2" and . "github.com/bsm/gomega") can make the code less readable and harder to understand dependencies. It's recommended to use standard import statements.
  • Update the Redis client import to the latest version (github.com/go-redis/redis/v8) as per your project requirements.

  1. Testing Framework: The code appears to be using the Ginkgo testing framework. Make sure you have the necessary setup to run Ginkgo tests and associated reports.

  2. Test Case Design:

  • The test cases seem reasonable and cover different scan types (Scan, ScanType, SScan, HScan, ZScan).
  • Consider adding more specific assertions within each test case to verify the expected behaviors. For example, checking specific keys/values returned by the scan commands.
  • It's also good practice to include negative test cases to validate error scenarios.
  1. Test Execution Environment: Ensure that the code executes in an appropriate testing environment with Redis accessible and properly configured.

Note: A code review is typically more effective when reviewing the entire context, including other relevant files, libraries used, and project-specific requirements.

Copy link

Choose a reason for hiding this comment

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

Overall, the code patch appears to be a test suite for scanning commands in a Redis database using the Pika integration library. Here are some observations and suggestions for improvement:

  1. Package Name: It is recommended to use lowercase letters for package names to follow Go naming conventions. Consider renaming "package pika_integration" to "package pikaintegration".

  2. Import Statements: The import statements contain both dot-imports ("." imports) and version-specific imports. It is generally discouraged to use dot-imports as they can make the code harder to read and understand. Instead, import the packages explicitly without the dot. Also, consider using the latest version of the imported packages if there have been updates since the code was last reviewed.

  3. Test Setup and Teardown: The BeforeEach and AfterEach functions provide setup and teardown logic for each test case. It's good to ensure that the tests start with a clean state and close any resources after execution.

  4. Test Cases: The code includes multiple test cases for different scan commands such as Scan, ScanType, SScan, HScan, and ZScan. Each test case performs some operations on the Redis database and verifies the results using Expect assertions. This approach is reasonable for testing the functionality of scanning commands.

  5. Error Handling: The code handles errors by using Expect functions from the Gomega library to assert that an error does not occur. However, it could be beneficial to include informative error messages when an error occurs. This would help with identifying the cause of the failure during test execution.

  6. Test Data: The test cases populate the Redis database with test data before executing the scan commands. It might be helpful to include additional scenarios with different types of data sets to cover a wider range of possible edge cases.

  7. Test Coverage: Verify if all relevant scenarios related to the scanning commands have been covered in the test cases. Ensure that both successful and failure cases are included.

  8. Code Documentation: Consider adding comments or function/method documentation to describe the purpose of the code patch, the role of individual functions, and any important details that might be useful for maintenance or future modifications.

Remember to validate this code review independently before applying any suggested changes, as I can only provide a general assessment based on the provided code snippet.

Loading