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:add macos test #2943

Closed
wants to merge 8 commits into from
Closed

feat:add macos test #2943

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
cb60cfe
feat:add macos test
brother-jin Nov 4, 2024
c36fd12
fixci
brother-jin Nov 5, 2024
caebd7a
fixci
brother-jin Nov 5, 2024
bb02d3a
fixci
brother-jin Nov 5, 2024
087cd0c
fixci
brother-jin Nov 5, 2024
e1babbf
fixci
brother-jin Nov 5, 2024
f59c753
fixci
brother-jin Nov 5, 2024
04d5d0e
fixci
brother-jin Nov 5, 2024
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
2 changes: 1 addition & 1 deletion .github/workflows/pika.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,7 @@ jobs:
run: |
cd tests/integration/
chmod +x integrate_test.sh
# sh integrate_test.sh
sh integrate_test.sh

build_pika_image:
name: Build Pika Docker image
Expand Down
5 changes: 4 additions & 1 deletion tests/integration/start_codis.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,10 @@ sed -i '' -e 's|databases : 1|databases : 2|' -e 's|port : 9221|port : 8001|' -e
#ensure both master and slave are ready
sleep 10

cd ../codis
pwd
echo "$(pwd)"

cd ../../codis
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

⚠️ Potential issue

The review comment's concerns are valid but need adjustment

The script is part of an integration test setup where the codis directory exists at the correct relative path from the test directory. However, error handling should still be added for robustness.

Apply this modified diff that preserves the test context:

-cd ../../codis
-make
+if ! cd ../../codis; then
+    echo "Error: Failed to change to codis directory from $(pwd)" >&2
+    exit 1
+fi
+
+if ! make; then
+    echo "Error: Codis build failed" >&2
+    exit 1
+fi
🔗 Analysis chain

Add error handling for directory navigation and build operations.

The current implementation has several reliability issues:

  1. No handling of cd command failure
  2. No verification of make command success
  3. Using relative path without validation

Apply this diff to improve reliability:

-cd ../../codis
-make
+if ! cd ../../codis; then
+    echo "Error: Failed to change directory to codis" >&2
+    exit 1
+fi
+
+if ! make; then
+    echo "Error: Failed to build codis" >&2
+    exit 1
+fi

Let's verify the codis directory structure:

Let me verify the repository structure to better understand the context.

Let me check the content of the start_codis.sh script to understand its context better.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Verify the expected directory structure exists
if [ -d "../../codis" ]; then
    echo "Codis directory found"
    ls -la ../../codis
else
    echo "Warning: Codis directory not found at expected location"
fi

Length of output: 145

Script:

#!/bin/bash
# Check current directory and list contents
pwd
echo "Current directory contents:"
ls -la

# Find any directory named codis
echo -e "\nSearching for codis directory:"
fd -t d codis

# Check the test file location
echo -e "\nLocating start_codis.sh:"
fd start_codis.sh

Length of output: 2762

Script:

#!/bin/bash
# Check the content and location of start_codis.sh
cat tests/integration/start_codis.sh

# Check if there's a Makefile in the codis directory
ls -la codis/Makefile

Length of output: 2713

🧰 Tools
🪛 Shellcheck

[warning] 35-35: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.

(SC2164)

make

echo 'startup codis dashboard and codis proxy'
Expand Down
Loading