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

feat: Supports Centos on CI #1772

Merged
merged 7 commits into from
Jul 21, 2023
Merged

feat: Supports Centos on CI #1772

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
4f26516
support centos on ci
Mixficsol Jul 20, 2023
9b6fa1b
Merge branch 'unstable' of github.com:OpenAtomFoundation/pika into at…
Mixficsol Jul 20, 2023
fe61a29
add unit test on centos
Mixficsol Jul 20, 2023
91c3345
Merge branch 'unstable' of github.com:OpenAtomFoundation/pika into at…
Mixficsol Jul 20, 2023
1669d94
find path
Mixficsol Jul 21, 2023
a23fa93
Merge branch 'unstable' of github.com:OpenAtomFoundation/pika into at…
Mixficsol Jul 21, 2023
95d463e
fix PythonE2E on centos
Mixficsol Jul 21, 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
55 changes: 34 additions & 21 deletions .github/workflows/pika.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ env:
BUILD_TYPE: RelWithDebInfo

jobs:
build:
build_on_ubuntu:
# The CMake configure and build commands are platform agnostic and should work equally well on Windows or Mac.
# You can convert this to a matrix build if you need cross-platform coverage.
# See: https://docs.github.com/en/free-pro-team@latest/actions/learn-github-actions/managing-complex-workflows#using-a-build-matrix
Expand Down Expand Up @@ -86,49 +86,62 @@ jobs:
run: |
yum install -y wget git autoconf centos-release-scl
yum install -y devtoolset-10-gcc devtoolset-10-gcc-c++ devtoolset-10-make devtoolset-10-bin-util
yum install -y llvm-toolset-7
yum install -y llvm-toolset-7-clang
yum install -y llvm-toolset-7 llvm-toolset-7-clang tcl unzip which python3
python3 -m pip install --upgrade pip
python3 -m pip install redis
source /opt/rh/devtoolset-10/enable
gcc --version
make --version

python3 --version

- name: Install cmake
run: |
wget https://github.com/Kitware/CMake/releases/download/v3.26.4/cmake-3.26.4-linux-x86_64.sh
bash ./cmake-3.26.4-linux-x86_64.sh --skip-license --prefix=/usr
cmake --version

- name: checkout
working-directory: ${{github.workspace}}
run: |
echo Fetching $GITHUB_REPOSITORY@$GITHUB_SHA
git init
git fetch --depth 1 https://github.com/$GITHUB_REPOSITORY $GITHUB_SHA
git checkout $GITHUB_SHA
- name: Checkout
uses: actions/checkout@v3
with:
fetch-depth: 0

- name: Configure CMake
# Configure CMake in a 'build' subdirectory. `CMAKE_BUILD_TYPE` is only required if you are using a single-configuration generator such as make.
# See https://cmake.org/cmake/help/latest/variable/CMAKE_BUILD_TYPE.html?highlight=cmake_build_type
run: |
source /opt/rh/devtoolset-10/enable
cmake -B ${{github.workspace}}/build -DCMAKE_BUILD_TYPE=${{env.BUILD_TYPE}} -DUSE_PIKA_TOOLS=ON
cmake -B build -DCMAKE_BUILD_TYPE=${{env.BUILD_TYPE}} -DUSE_PIKA_TOOLS=ON

- name: Build
# Build your program with the given configuration
run: |
cd ${{github.workspace}}
source /opt/rh/devtoolset-10/enable
cmake --build ${{github.workspace}}/build --config ${{env.BUILD_TYPE}}
cmake --build build --config ${{env.BUILD_TYPE}}

- name: Test
# Execute tests defined by the CMake configuration.
# See https://cmake.org/cmake/help/latest/manual/ctest.1.html for more detail
run: |
cd ${{github.workspace}}/build
source /opt/rh/devtoolset-10/enable
ctest -C ${{env.BUILD_TYPE}}

- name: Unit Test
run: |
chmod +x pikatests.sh
sh pikatests.sh all

# master on port 9221, slave on port 9231, all with 2 db
- name: Start pika master and slave
run: |
cd build
chmod +x ../tests/integration/start_master_and_slave.sh
sh ../tests/integration/start_master_and_slave.sh

- name: Run Python E2E Tests
run: |
python3 tests/integration/pika_replication_test.py
python3 tests/unit/Blpop_Brpop_test.py

build_on_macos:
runs-on: macos-latest
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 code review suggestions for the given code patch:

  1. It seems that a new job called build_on_ubuntu has been added, while the existing build job is commented out. Make sure this change aligns with your intended workflow.

  2. In the Upload workspace artifact step, you can consider specifying a different path for the uploaded artifact to avoid potential conflicts with existing files or directories. For example, instead of using the root directory, you can specify a subdirectory within the artifact space.

  3. The commented-out Test step can be re-enabled and modified to execute the desired test command after the build. Adjust the commands to match your project's test setup.

  4. In the Download workspace artifact step, you may want to consider providing a specific version or using a unique name for the downloaded artifact to avoid using conflicting or outdated artifacts.

  5. If the symbolic link creation in the Create build soft link step is necessary for subsequent steps, make sure it points to the correct target location. Verify the paths and adjust accordingly.

  6. In the Unzip workspace directory step, consider specifying a destination directory other than /github/workspace/artifacts. Using a dedicated directory for unzipping might prevent conflicts with existing files or directories.

  7. In the Unit Test step, make sure that the paths and directories accessed by the commands are accurate and point to the correct locations. Double-check the paths and adjust if needed.

  8. Ensure that all required dependencies and packages are installed properly in each platform-specific block (run) based on your project's needs.

Remember to thoroughly test the workflow after implementing these changes to ensure everything works as expected.

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 bug risks and improvement suggestions for the code patch:

  1. In the step Zip workspace directory, the command should include the -q flag to run the zip command quietly:

    run: zip -r -q workspace.zip ${{ github.workspace }}

  2. In the step install Deps, ensure that the script pikatests.sh is executable before running it by adding the following command before sh pikatests.sh all:

    chmod +x pikatests.sh

  3. In the commented-out Test step, make sure to uncomment it if you want to execute the tests defined by the CMake configuration.

  4. In the step Create artifact directory, change the path to:

    run: mkdir -p /github/workspace/artifacts/pika/pika/build

  5. In the step Download workspace artifact, specify the correct name of the artifact to download:

    with:
  name: workspace-artifact

  6. In the step 创建 build 软链接, change the command as follows:

    run: ln -s /github/workspace/artifacts/pika/pika/build /github/workspace/artifacts/home/runner/work/pika/pika/build

  7. The step Unzip workspace directory should use the correct artifact path:

    run: unzip -q /github/workspace/workspace-artifact.zip -d /github/workspace/artifacts

  8. In the step Unit Test, change the command to navigate to the correct directory before running the tests:

    cd /github/workspace/artifacts/pika/pika/

These are suggestions based on the provided code, but please note that a more thorough review may require understanding the overall context and requirements of your project.


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 looks good. Here are a few suggestions and improvements:

  1. Add comments: Although there are some comments explaining the purpose of certain actions, it would be helpful to add more comments throughout the workflow to enhance readability and make it easier for others to understand.

  2. Use specific job names: Instead of using generic names like build_on_ubuntu and build_on_macos, consider using specific names that reflect the purpose of each job. This will make it easier to identify and understand the different jobs in the workflow.

  3. Consider matrix builds: The comment suggests converting the build into a matrix build if cross-platform coverage is needed. This could be a good improvement, especially if you want to test the code on multiple platforms efficiently.

  4. Check if zip program is available: Before running the zip command, you might want to check if the zip program is available in the environment. You can use the which command to verify its existence before proceeding with zipping.

  5. Error handling: Ensure proper error handling in case any of the steps fail by using set -e at the beginning of your shell script. This will cause the script to exit immediately if any command fails, preventing further execution of potentially erroneous commands.

  6. Validation for the artifact download: After unzipping the workspace artifact, consider adding validation steps such as verifying specific files or running additional checks to ensure the artifact was downloaded and extracted correctly.

  7. Add artifact retention policy: Consider adding an artifact retention policy to specify how long the artifacts should be retained. This will prevent consuming excessive storage space over time.

Remember to test the updated workflow thoroughly to ensure all the modifications work as expected.

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 includes several improvements and bug fixes. Here are some suggestions for further improvement:

  1. Use more descriptive names for jobs: The job names should reflect their purpose or the specific platform being used (e.g., build_on_ubuntu and build_on_macos).

  2. Check for errors during the execution of commands: It's a good practice to add error handling for individual commands and steps in case they fail. This can prevent failures from propagating further in the workflow.

  3. Consider using environment variables instead of hardcoding paths: Instead of hardcoded paths like /home/runner/work/pika/pika/build, you could use environment variables provided by the GitHub Actions runtime. For example, $GITHUB_WORKSPACE represents the path of the workspace directory.

  4. Avoid unnecessary steps: Some steps may not be essential or can be combined if they serve a similar purpose. Review each step to ensure clarity and efficiency.

  5. Ensure consistent indentation: Make sure the indentation is consistent throughout the code to improve readability and avoid accidental errors.

  6. Add comments for clarity: Add comments to explain the purpose and functionality of particular steps or sections to aid understanding for future readers and maintainers.

  7. Consider isolating unit tests in a separate job: It might be beneficial to separate the unit test step into its own job to isolate and parallelize testing if feasible.

  8. Verify expected file presence and contents: After unzipping the workspace artifact, consider adding checks to verify that the expected files are present and have the correct content before performing subsequent actions.

Remember, these suggestions are based on a superficial review of the code patch. It's always important to thoroughly test the changes and consider the specific requirements and constraints of your project.

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 looks fine, but there are a few bug risks and improvement suggestions:

  1. In the job name change, consider using a more descriptive name like "build_on_ubuntu" to make it clear that this job is specifically for Ubuntu.

  2. When zipping the workspace directory, make sure to exclude any unnecessary or sensitive files or directories that shouldn't be included in the artifact.

  3. You can improve the organization of the job by separating the steps into logical sections with comments. This makes it easier to understand and maintain the workflow.

  4. Consider using the cache action for caching dependencies to speed up subsequent runs. It appears that the cache step is used for caching pip dependencies, but you could also explore caching other dependencies if applicable.

  5. Make sure to handle error conditions appropriately. For example, check if the artifact download is successful before proceeding with subsequent steps.

  6. Use environment variables instead of hardcoding paths where possible (e.g., /opt/rh/devtoolset-10/enable).

  7. Add error handling and feedback mechanisms for each step in the workflow, so you can easily troubleshoot and identify failures.

  8. Consider using matrix builds to run the same job on different versions of Ubuntu and/or configurations for better coverage. This allows you to test your code on multiple setups efficiently.

These suggestions aim to improve code readability, maintainability, and overall reliability of the workflow. It's important to thoroughly test and validate the workflow changes before applying them to production environments.

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 seems to have some improvements and bug fixes. Here are a few suggestions for further improvement:

  1. Provide more descriptive names for the jobs: Instead of build_on_ubuntu and build_on_macos, consider using more meaningful names that describe the purpose of each job. This can help with readability and understanding the workflow.

  2. Add error handling and resilience: It would be beneficial to add error handling and resilience mechanisms in case any command or step fails. For example, you could use try-catch blocks or conditional statements to handle potential failures during artifact downloading or unzipping.

  3. Review dependencies installation: Ensure that all necessary dependencies are correctly listed and required versions are specified. Double-check if there are any additional dependencies required for successful execution.

  4. Simplify paths and directory structure: If possible, avoid using hardcoded paths and create a more flexible and generic directory structure. This can make the code more portable and adaptable to different environments.

  5. Add comments: Although the code is relatively self-explanatory, it's always helpful to add comments explaining the purpose of specific steps or sections to improve code understandability and maintainability.

  6. Consider using matrix builds: If cross-platform coverage is a requirement, consider converting the workflow into a matrix build. This allows running the same set of steps on multiple platforms without duplicating the code.

  7. Test error scenarios: It's essential to test potential error scenarios, such as the failure of artifact download or unexpected behavior in test scripts. Include tests or checks to ensure your workflow handles these situations gracefully.

Remember to thoroughly test the updated workflow to verify its correctness and effectiveness.

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 looks fine, but there are a few suggestions and improvements that can be made:

  1. Job Name: Instead of renaming the existing build job to build_on_ubuntu, it might be better to create a new job with the desired name and keep the original build job for compatibility reasons.

  2. Artifact Management:

  • The step to zip the entire workspace directory and upload it as an artifact is good. However, you should consider excluding unnecessary files or directories from being included in the zip file.
  • In the subsequent job, you're downloading the artifact and unzipping it. Instead of downloading and unzipping, you can specify the artifact path directly using the path parameter in the Download workspace artifact step. This way, you don't need to manually unzip the workspace.
  • Make sure to cleanup or delete the workspace.zip after artifact upload and extraction, as it might take up unnecessary space.
  1. Dependency Installation:
  • Consider using package managers like apt-get instead of yum in the Ubuntu job. apt-get is commonly used for package management on Ubuntu-based systems.
  • In the CentOS job, make sure to use yum update before installing other dependencies to ensure the system packages are up to date.
  • It's good to upgrade pip before installing dependencies, but you may want to specify a specific version to avoid potential compatibility issues if a major pip version update occurs.
  1. Test Execution:
  • In the Unit Test step, when moving the build directory, ensure that the destination directory doesn't already exist to prevent overwriting any existing data.
  • When running tests, provide the necessary configuration or command line arguments required for successful execution.
  • For the E2E tests, it's recommended to include specific assertions or checks to verify the expected outcomes of the tests.

These suggestions are aimed at improving code clarity and reliability. Remember to thoroughly test the code patch after making these changes to ensure it works as expected.

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. Job Names: It's a good practice to use descriptive names for jobs. Instead of renaming the existing "build" job, you added a new job named "build_on_ubuntu." Consider giving it a more specific name that reflects its purpose or target platform.

  2. Zip and Artifact Handling: The addition of zipping the workspace directory and uploading it as an artifact is a good improvement. You can simplify the process by using the actions/upload-artifact action directly after the zipping step, without creating a separate job for it. No need to download and unzip it again in subsequent jobs. You can access the artifact directly in subsequent jobs.

  3. Installation of Dependencies: You're installing additional packages (which, tcl, unzip, python3) and upgrading pip. These steps seem fine, but ensure that these dependencies are necessary for your project and version compatibility.

  4. Caching Dependencies: Caching installed dependencies can improve build times. Make sure the caching mechanism works correctly by testing it thoroughly.

  5. Improving Readability: Add comments to clarify the purpose of each step, job, or group of commands. This helps other developers (and even yourself) understand the code better.

  6. Unit Tests and Integration Tests: There seems to be a section commented out that appears to handle unit tests and integration tests. Depending on what you're aiming for, you might want to uncomment and define the usage of these tests explicitly.

  7. Remove Unused Section: The last empty section (build_on_macos) does not contain any steps or configurations. You can remove it from the code snippet.

It's important to note that a complete code review would require analyzing the context and purpose of the entire workflow where this code patch is being integrated. This assessment is based solely on the provided code patch.

Expand Down Expand Up @@ -161,11 +174,11 @@ jobs:
cmake -B ${{github.workspace}}/build -S . -DCMAKE_C_COMPILER=/usr/local/opt/gcc@10/bin/gcc-10 -DCMAKE_BUILD_TYPE=${{env.BUILD_TYPE}}
cmake --build ${{github.workspace}}/build --config ${{env.BUILD_TYPE}}

- name: Test
working-directory: ${{github.workspace}}/build
run: |
export CC=/usr/local/opt/gcc@10/bin/gcc-10
ctest -C ${{env.BUILD_TYPE}}
- name: Test
working-directory: ${{github.workspace}}/build
run: |
export CC=/usr/local/opt/gcc@10/bin/gcc-10
ctest -C ${{env.BUILD_TYPE}}

- name: Unit Test
working-directory: ${{github.workspace}}
Expand All @@ -176,7 +189,7 @@ jobs:
- name: Start pika master and slave
working-directory: ${{github.workspace}}/build
run: |
cd ${{github.workspace}}/build
cd ${{github.workspace}}/build
chmod +x ../tests/integration/start_master_and_slave.sh
../tests/integration/start_master_and_slave.sh

Expand Down
26 changes: 13 additions & 13 deletions tests/integration/pika_replication_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ def test_del_replication():

master.close()
slave.close()
print("Del multiple keys replication OK [✓]")
print("Del multiple keys replication OK [Passed]")


def test_msetnx_replication():
Expand Down Expand Up @@ -180,7 +180,7 @@ def random_mset_thread(keys_):
m_v = master.get(key)
s_v = slave.get(key)
assert m_v == s_v, f'Expected: master_v == slave_v, but got slave_v:{s_v}, master_v:{m_v}, using key:{key}'
print("test_msetnx_replication OK [✓]")
print("test_msetnx_replication OK [Passed]")


def test_mset_replication():
Expand Down Expand Up @@ -245,7 +245,7 @@ def random_mset_thread(keys_):
m_v = master.get(key)
s_v = slave.get(key)
assert m_v == s_v, f'Expected: master_v == slave_v, but got slave_v:{s_v}, master_v:{m_v}, using key:{key}'
print("test_mset_replication OK [✓]")
print("test_mset_replication OK [Passed]")


def test_smove_replication():
Expand Down Expand Up @@ -296,7 +296,7 @@ def random_smove_thread():

assert m_source_set == s_source_set, f'Expected: source_set on master == source_set on slave, but got source_set on slave:{s_source_set}, source_set on master:{m_source_set}'
assert m_dest_set == s_dest_set, f'Expected: dest_set on master == dest_set on slave, but got dest_set on slave:{s_dest_set}, dest_set on master:{m_dest_set}'
print("start test_smove_replication OK [✓]")
print("start test_smove_replication OK [Passed]")


def test_rpoplpush_replication():
Expand Down Expand Up @@ -378,7 +378,7 @@ def rpoplpush_thread1():
# print(slave.lindex('blist', i))
assert master.lindex('blist', i) == slave.lindex('blist', i), \
f"Expected:master.lindex('blist', i) == slave.linex('blist', i), but got False when i = {i}"
print("test_rpoplpush_replication OK [✓]")
print("test_rpoplpush_replication OK [Passed]")


def test_sdiffstore_replication():
Expand Down Expand Up @@ -443,7 +443,7 @@ def random_sdiffstore_thread():
assert m_set1 == s_set1, f'Expected: set1 on master == set1 on slave, but got set1 on slave:{s_set1}, set1 on master:{m_set1}'
assert m_set2 == s_set2, f'Expected: set2 on master == set2 on slave, but got set2 on slave:{s_set2}, set2 on master:{m_set2}'
assert m_dest_set == s_dest_set, f'Expected: dest_set on master == dest_set on slave, but got dest_set on slave:{s_dest_set}, dest_set on master:{m_dest_set}'
print("test_sdiffstore_replication OK [✓]")
print("test_sdiffstore_replication OK [Passed]")


def test_sinterstore_replication():
Expand Down Expand Up @@ -505,7 +505,7 @@ def random_sinterstore_thread():
s_dest_set = slave.smembers('dest_set')

assert m_dest_set == s_dest_set, f'Expected: dest_set on master == dest_set on slave, but got dest_set on slave:{s_dest_set}, dest_set on master:{m_dest_set}'
print("test_sinterstore_replication OK [✓]")
print("test_sinterstore_replication OK [Passed]")


def test_zunionstore_replication():
Expand Down Expand Up @@ -559,7 +559,7 @@ def random_zunionstore_thread():
s_zset_out = slave.zrange('zset_out', 0, -1, withscores=True)

assert m_zset_out == s_zset_out, f'Expected: zset_out on master == zset_out on slave, but got zset_out on slave:{s_zset_out}, zset_out on master:{m_zset_out}'
print("test_zunionstore_replication OK [✓]")
print("test_zunionstore_replication OK [Passed]")


def test_zinterstore_replication():
Expand Down Expand Up @@ -621,7 +621,7 @@ def random_zinterstore_thread():

assert m_zset_out == s_zset_out, f'Expected: zset_out on master == zset_out on slave, but got zset_out on slave:{s_zset_out}, zset_out on master:{m_zset_out}'

print("test_zinterstore_replication OK [✓]")
print("test_zinterstore_replication OK [Passed]")


def test_sunionstore_replication():
Expand Down Expand Up @@ -676,7 +676,7 @@ def random_sunionstore_thread():
s_set_out = slave.smembers('set_out')

assert m_set_out == s_set_out, f'Expected: set_out on master == set_out on slave, but got set_out on slave:{s_set_out}, set_out on master:{m_set_out}'
print("test_sunionstore_replication OK [✓]")
print("test_sunionstore_replication OK [Passed]")


def test_bitop_replication():
Expand Down Expand Up @@ -729,7 +729,7 @@ def random_bitop_thread():

assert m_key_out_count1 == s_key_out_count1, f'Expected: bitcount of bitkey_out1 on master == bitcount of bitkey_out1 on slave, but got bitcount of bitkey_out1 on slave:{s_key_out_count1}, bitcount of bitkey_out1 on master:{m_key_out_count1}'
assert m_key_out_count2 == s_key_out_count2, f'Expected: bitcount of bitkey_out2 on master == bitcount of bitkey_out2 on slave, but got bitcount of bitkey_out2 on slave:{s_key_out_count2}, bitcount of bitkey_out1 on master:{m_key_out_count2}'
print("test_bitop_replication OK [✓]")
print("test_bitop_replication OK [Passed]")


def test_pfmerge_replication():
Expand Down Expand Up @@ -779,7 +779,7 @@ def random_pfmerge_thread():
s_hll_out = slave.pfcount('hll_out')

assert m_hll_out == s_hll_out, f'Expected: hll_out on master == hll_out on slave, but got hll_out on slave:{s_hll_out}, hll_out on master:{m_hll_out}'
print("test_pfmerge_replication OK [✓]")
print("test_pfmerge_replication OK [Passed]")

def test_migrateslot_replication():
print("start test_migrateslot_replication")
Expand Down Expand Up @@ -887,7 +887,7 @@ def test_migrateslot_replication():

i_keys = master.keys("_internal:slotkey:4migrate*")
master.delete(*i_keys)
print("test_migrateslot_replication OK [✓]")
print("test_migrateslot_replication OK [Passed]")

master_ip = '127.0.0.1'
master_port = '9221'
Expand Down
16 changes: 8 additions & 8 deletions tests/unit/Blpop_Brpop_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ def test_single_existing_list(db_):
assert result[0] == b'blist' and result[1] == b'b', f"Expected (b'blist1', b'b'), but got {result}"

pika.close()
print("test_single_existing_list Passed [✓], db:db%d" % (db_))
print("test_single_existing_list Passed [Passed], db:db%d" % (db_))


# 解阻塞测试（超时自动解阻塞，lpush解阻塞，rpush解阻塞，rpoplpush解阻塞）
Expand Down Expand Up @@ -213,7 +213,7 @@ def brpop_thread51():
assert blocked == False, f"Expected False but got {blocked}"
thread.join()
pika.close()
print("test_blpop_brpop_unblock_lrpush_rpoplpush Passed [✓], db:db%d" % (db_))
print("test_blpop_brpop_unblock_lrpush_rpoplpush Passed [Passed], db:db%d" % (db_))


def test_concurrency_block_unblock(db_):
Expand Down Expand Up @@ -350,7 +350,7 @@ def rpush_thread(list_, value_):
t.join()
pika.delete('blist0', 'blist1', 'blist2', 'blist3')

print("test_concurrency_block_unblock Passed [✓], db:db%d" % (db_))
print("test_concurrency_block_unblock Passed [Passed], db:db%d" % (db_))
pika.close()


Expand Down Expand Up @@ -396,7 +396,7 @@ def test_multiple_existing_lists(db_):
assert result[0] == b'blist1' and result[1] == b'large', f"Expected (b'blist1', b'large'), but got {result}"

pika.close()
print("test_multiple_existing_lists Passed [✓], db:db%d" % (db_))
print("test_multiple_existing_lists Passed [Passed], db:db%d" % (db_))


def test_blpop_brpop_same_key_multiple_times(db_):
Expand Down Expand Up @@ -488,7 +488,7 @@ def brpop_thread2():
assert result[0] == b'list2' and result[1] == b'd', f"Expected (b'list2', b'd'), but got {result}"

pika.close()
print("test_blpop_brpop_same_key_multiple_times Passed [✓], db:db%d" % (db_))
print("test_blpop_brpop_same_key_multiple_times Passed [Passed], db:db%d" % (db_))


# 目标list被一条push增加了多个value，先完成多个value的入列再pop
Expand Down Expand Up @@ -542,7 +542,7 @@ def brpop_thread():
thread.join()
# 检查blist的第一个元素
assert pika.lindex('blist', 0) == b'foo', "Expected 'foo'"
print("test_blpop_brpop_variadic_lpush Passed [✓], db:db%d" % (db_))
print("test_blpop_brpop_variadic_lpush Passed [Passed], db:db%d" % (db_))


# 先被阻塞的先服务/阻塞最久的优先级最高
Expand Down Expand Up @@ -587,7 +587,7 @@ def brpop_thread(expect):
t4.join()

pika.close()
print("test_serve_priority Passed [✓], db:db%d" % (db_))
print("test_serve_priority Passed [Passed], db:db%d" % (db_))


# 主从复制测试
Expand Down Expand Up @@ -813,7 +813,7 @@ def rpush_thread(list_, value_, value2_, value3_, value4_, value5_):

master.close()
slave.close()
print("test_master_slave_replication Passed [✓], db:db%d" % (db_))
print("test_master_slave_replication Passed [Passed], db:db%d" % (db_))

def test_with_db(db_id):
test_master_slave_replication(db_id)
Copy link

Choose a reason for hiding this comment

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

Here are a few suggestions to improve the code:

  1. Use descriptive variable names: Instead of using single-letter variable names like db_, list_, and value_, consider using more descriptive names such as database_number, list_name, and item_value. This will make the code easier to understand and maintain.

  2. Remove unnecessary comments: Comments like # 解阻塞测试（超时自动解阻塞，lpush解阻塞，rpush解阻塞，rpoplpush解阻塞） can be removed or translated into English for better readability, depending on your target audience.

  3. Consistent naming convention: Make sure to use consistent naming conventions throughout the code. For example, you have both test_blpop_brpop_unblock_lrpush_rpoplpush and brpop_thread51. It's good practice to follow a naming convention such as snake_case for function and variable names to improve code readability.

  4. Use f-strings for formatting: Instead of using string concatenation with % operator, consider using f-strings for better readability and simpler syntax. For example, print("test_single_existing_list Passed [Passed], db:db%d" % (db_)) can be written as print(f"test_single_existing_list Passed [Passed], db:db{db_}").

  5. Avoid hardcoded values: In functions like rpush_thread and rpush_thread2, instead of hardcoding the list names ('blist0', 'blist1', 'blist2', 'blist3'), consider passing them as arguments to the function. This will make the code more flexible and reusable.

  6. Add exception handling: Consider adding appropriate exception handling throughout the code to handle potential errors or exceptions that may occur during execution. This will make the code more robust.

Overall, the code appears to be mostly focused on printing some success messages and assertions. Without a complete understanding of the context and requirements, it's difficult to identify specific bug risks or suggest further improvements. It's recommended to thoroughly test the code with different scenarios to ensure its correctness and efficiency.

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 seems to be mostly focused on modifying print statements to change the output messages. There don't appear to be any bug risks or improvement suggestions based on the provided context.

However, here are a few general good practices for code review:

  1. Use meaningful variable and function names: Ensure that variable and function names accurately describe their purpose or functionality. This improves code readability and maintainability.

  2. Add comments: Consider adding comments to explain complex sections of code or provide additional context where necessary. This can make it easier for others (and even yourself) to understand the code later on.

  3. Maintain consistent code style: Consistent indentation, spacing, and formatting improve the code's readability. Ensure that the code adheres to a specific style guide or convention consistently throughout.

  4. Check error handling: Verify if error handling mechanisms (like try-except blocks) are implemented correctly and handle potential exceptions gracefully to prevent unexpected crashes or behavior.

Remember, without the full context of the code and its purpose, it's difficult to provide specific improvements or identify potential bugs. It's always a good practice to thoroughly test your code with different scenarios to ensure its correctness and reliability.

Copy link

Choose a reason for hiding this comment

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

Based on the code patch, here are some observations and suggestions:

  1. The changes made in the patch are purely cosmetic, updating the print statements to change "[✓]" to "[Passed]". These changes don't affect the functionality or correctness of the code.

  2. It's recommended to add more comments or docstrings to explain the purpose of each function and any significant logic within the code. This will make it easier for future developers to understand and maintain the code.

  3. Consider using a logging library instead of print statements for better log management and flexibility. The logging module in Python provides a more robust and configurable way to handle logging.

  4. Reviewing only this code patch without seeing the rest of the codebase limits the potential identification of bugs and improvements. For a more comprehensive review, it would be helpful to inspect the entire codebase.

Copy link

Choose a reason for hiding this comment

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

Based on the provided code patch, here are some potential bug risks and improvement suggestions:

  1. In the test_single_existing_list function, the assert statement comparing the expected and actual results could be written in a more concise way using tuple unpacking:

    assert result == (b'blist', b'b'), f"Expected (b'blist', b'b'), but got {result}"

  2. In the brpop_thread51 function, there is no need for the additional assert statement after joining the thread. You can safely remove it.

  3. In the rpush_thread function, you should consistentl

Expand Down