Fixed workflow tests. #2073
Fixed workflow tests. #2073
Conversation
|
Warning Rate limit exceeded@AlexSkrypnyk has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 minutes and 5 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThis PR modifies test infrastructure by updating CI workflows to support grouped PHPUnit execution with SSH key generation and git configuration setup, removes comprehensive BATS test helper files (workflow orchestration and fixture utilities), adds a PHPUnit test group attribute, and eliminates many helper functions from the BATS helper suite. Changes
Sequence Diagram(s)sequenceDiagram
participant CI as CI Runner
participant SSH as SSH Setup
participant Git as Git Config
participant Network as Docker Network
participant PHPUnit as PHPUnit (Grouped)
CI->>SSH: Generate SSH key & export SSH_AUTH_SOCK
activate SSH
SSH-->>CI: SSH initialized
deactivate SSH
CI->>Git: Set global user.name & user.email (if absent)
activate Git
Git-->>CI: Git configured
deactivate Git
CI->>Network: Create amazezeio-network
activate Network
Network-->>CI: Network ready
deactivate Network
CI->>PHPUnit: Run phpunit group="p${TEST_NODE_INDEX:-*}"
activate PHPUnit
PHPUnit-->>CI: Test results
deactivate PHPUnit
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #2073 +/- ##
========================================
Coverage 54.74% 54.74%
========================================
Files 92 92
Lines 5692 5692
Branches 44 44
========================================
Hits 3116 3116
Misses 2576 2576 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
AlexSkrypnyk
force-pushed
the
feature/fix-workflow-tests
branch
from
98b3dbd to
fdb458c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/vortex-test-common.yml (1)
53-60: Critical: SSH key is not added to ssh-agent, causing SSH operations to fail.The SSH key pair is generated and ssh-agent is started, but the private key is never added to the agent using
ssh-add. This meansSSH_AUTH_SOCKis set to point to an agent with no identities loaded. Any SSH-based operation (e.g., git operations over SSH, deployment tests) will fail because the agent cannot provide the key.Add
ssh-add "$HOME/.ssh/id_rsa"after starting the agent to load the key:ssh-agent -a "${HOME}/ssh-agent.sock" > ssh-agent-output export SSH_AUTH_SOCK=$(grep SSH_AUTH_SOCK ssh-agent-output | cut -d';' -f1 | cut -d= -f2) echo "SSH_AUTH_SOCK=${SSH_AUTH_SOCK}" >> "$GITHUB_ENV" + ssh-add "$HOME/.ssh/id_rsa"Additionally, clean up the temporary output file:
ssh-agent -a "${HOME}/ssh-agent.sock" > ssh-agent-output export SSH_AUTH_SOCK=$(grep SSH_AUTH_SOCK ssh-agent-output | cut -d';' -f1 | cut -d= -f2) echo "SSH_AUTH_SOCK=${SSH_AUTH_SOCK}" >> "$GITHUB_ENV" ssh-add "$HOME/.ssh/id_rsa" + rm -f ssh-agent-outputAlso applies to: 144-151
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/vortex-test-common.yml(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: AlexSkrypnyk
Repo: drevops/vortex PR: 1816
File: .docker/cli.dockerfile:72-76
Timestamp: 2025-07-23T01:16:30.963Z
Learning: In the Vortex project, unauthenticated Composer installs should be allowed, so GitHub token secrets should not be marked as `required=true` in Docker build mounts. The conditional check `if [ -s /run/secrets/github_token ]` is the preferred approach to allow builds to proceed without a token when needed.
📚 Learning: 2025-08-08T12:02:24.652Z
Learnt from: AlexSkrypnyk
Repo: drevops/vortex PR: 1896
File: .vortex/tests/bats/unit/download-db-lagoon.bats:24-25
Timestamp: 2025-08-08T12:02:24.652Z
Learning: In .vortex/tests/bats/unit Bats tests using ../_helper.bash (run_steps), prefixing a STEPS entry with "- " denotes a negative assertion (the substring must NOT appear in output). Unprefixed entries are positive assertions. Example: "- Database dump refresh requested. Will create a new dump." asserts absence; "Database dump refresh requested. Will create a new dump." asserts presence.
Applied to files:
.github/workflows/vortex-test-common.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: build (1)
- GitHub Check: build (0)
- GitHub Check: vortex-test-workflow (4)
- GitHub Check: vortex-test-workflow (1)
- GitHub Check: vortex-test-workflow (3)
- GitHub Check: vortex-test-workflow (0)
- GitHub Check: vortex-test-workflow (2)
- GitHub Check: vortex-test-common
- GitHub Check: vortex-test-installer (8.4)
- GitHub Check: vortex-test-installer (8.3)
- GitHub Check: vortex-test-installer (8.2)
- GitHub Check: vortex-test-docs
| # Configure git username and email if it is not set. | ||
| [ "$(git config --global user.name)" = "" ] && echo "==> Configuring global test git user name" && git config --global user.name "Test user" | ||
| [ "$(git config --global user.email)" = "" ] && echo "==> Configuring global test git user email" && git config --global user.email "someone@example.com" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use -z flag for clearer zero-length string checks.
The git config guard logic is correct but can be made more idiomatic by using the -z flag instead of = "" comparisons, which is the standard POSIX shell pattern for checking empty strings.
- [ "$(git config --global user.name)" = "" ] && echo "==> Configuring global test git user name" && git config --global user.name "Test user"
- [ "$(git config --global user.email)" = "" ] && echo "==> Configuring global test git user email" && git config --global user.email "someone@example.com"
+ [ -z "$(git config --global user.name)" ] && echo "==> Configuring global test git user name" && git config --global user.name "Test user"
+ [ -z "$(git config --global user.email)" ] && echo "==> Configuring global test git user email" && git config --global user.email "someone@example.com"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Configure git username and email if it is not set. | |
| [ "$(git config --global user.name)" = "" ] && echo "==> Configuring global test git user name" && git config --global user.name "Test user" | |
| [ "$(git config --global user.email)" = "" ] && echo "==> Configuring global test git user email" && git config --global user.email "someone@example.com" | |
| # Configure git username and email if it is not set. | |
| [ -z "$(git config --global user.name)" ] && echo "==> Configuring global test git user name" && git config --global user.name "Test user" | |
| [ -z "$(git config --global user.email)" ] && echo "==> Configuring global test git user email" && git config --global user.email "someone@example.com" |
🤖 Prompt for AI Agents
.github/workflows/vortex-test-common.yml around lines 174 to 176: the current
checks use string equality to test for empty git config values; replace those
comparisons with the POSIX idiom using the -z test so the conditions read check
if the command substitution returns a zero-length string (e.g., [ -z "$(git
config --global user.name)" ] && ...), keeping the same echo and git config
assignment logic.
| docker network create amazeeio-network 2>/dev/null || true | ||
| php -d memory_limit=-1 vendor/bin/phpunit --group="p${TEST_NODE_INDEX:-*}" |
There was a problem hiding this comment.
🧩 Analysis chain
Docker network creation and phpunit grouping look sound.
The docker network stub creation with proper error suppression follows best practices, and the phpunit test grouping pattern p${TEST_NODE_INDEX:-*} correctly distributes tests across matrix batches (0–4). The fallback to * (all tests) for unset indices provides sensible defaults.
Verify that tests tagged with groups p0 through p4 exist and are properly distributed to ensure no test gaps or overlaps across batches:
🏁 Script executed:
#!/bin/bash
# Description: Verify phpunit test groups are properly defined and distributed
# Search for test group annotations in the test files
rg -n '@group\s+p[0-4]' .vortex/tests --type=php | head -20
# Count tests per group
echo "=== Test count per group ==="
rg '@group\s+p[0-4]' .vortex/tests --type=php -c | sort | uniq -cLength of output: 28
I need to search more broadly to understand the actual test grouping strategy used in this repository.
🏁 Script executed:
#!/bin/bash
# Description: Search for all @group annotations to understand actual test grouping
echo "=== All @group annotations in .vortex/tests ==="
rg -n '@group' .vortex/tests --type=php -A1 | head -30
echo ""
echo "=== Unique group names ==="
rg '@group\s+(\S+)' .vortex/tests --type=php -o --replace '$1' | sort | uniq -c | sort -rn
echo ""
echo "=== Check for phpunit.xml or test configuration ==="
fd '(phpunit|test)" -type f .vortex/tests | head -20Length of output: 198
Let me fix the script and gather more comprehensive information:
🏁 Script executed:
#!/bin/bash
# Description: Understand test structure and grouping
echo "=== Test files in .vortex/tests ==="
fd --type f '.php$' .vortex/tests | head -20
echo ""
echo "=== Search for @group in test files ==="
rg -n '@group' .vortex/tests --type=php | head -20
echo ""
echo "=== Check for phpunit.xml ==="
fd -name 'phpunit.xml*' .vortex
echo ""
echo "=== Check workflow context - lines around 179 ==="
sed -n '170,190p' .github/workflows/vortex-test-common.ymlLength of output: 2422
🏁 Script executed:
#!/bin/bash
# Description: Check phpunit config and TEST_NODE_INDEX usage
echo "=== Find phpunit configuration files ==="
find .vortex -name 'phpunit.xml*' -o -name 'phpunit.dist.xml' 2>/dev/null
echo ""
echo "=== Search for TEST_NODE_INDEX in workflow ==="
grep -n 'TEST_NODE_INDEX' .github/workflows/vortex-test-common.yml
echo ""
echo "=== Check matrix batch configuration ==="
sed -n '1,200p' .github/workflows/vortex-test-common.yml | grep -A 20 'strategy:'
echo ""
echo "=== Verify no @group tags exist at all in tests ==="
rg -c '^[[:space:]]*\*[[:space:]]*@group' .vortex/tests --type=phpLength of output: 1661
🏁 Script executed:
#!/bin/bash
# Description: Check phpunit.xml configuration
echo "=== phpunit.xml content ==="
cat .vortex/tests/phpunit.xml
echo ""
echo "=== Check if there are any group-related configurations ==="
grep -i 'group' .vortex/tests/phpunit.xml || echo "(no group configuration found)"Length of output: 1327
Matrix test distribution is non-functional—tests lack required group annotations.
The workflow references --group="p${TEST_NODE_INDEX:-*}" with matrix batches [0–4], but no tests in .vortex/tests are tagged with @group p0 through @group p4. The phpunit.xml contains no group configuration. Consequently, all batches execute the complete test suite instead of distributing work across parallel jobs, defeating the matrix strategy's purpose.
Tests must be annotated with @group p0, @group p1, etc., to enable proper parallelization. Without these annotations, the workflow will continue running redundant full test suites in each batch.
🤖 Prompt for AI Agents
.github/workflows/vortex-test-common.yml lines 179-181: the CI uses phpunit
--group="p${TEST_NODE_INDEX:-*}" to split tests across matrix nodes but no tests
are annotated with @group p0..p4 (and phpunit.xml has no group config), so each
matrix job runs the full suite; fix by annotating tests in .vortex/tests with
the appropriate group tags matching your matrix indices (e.g., add @group p0,
@group p1, … to test classes or methods) or alternatively update phpunit.xml to
define group aliases or implement a test-splitting script that assigns files to
groups consistently with the matrix; ensure the group names exactly match p0..p4
and run a local phpunit --group check to verify distribution.
4e9ffa4 to
53c8d3a
Compare
AlexSkrypnyk
force-pushed
the
feature/fix-workflow-tests
branch
from
53c8d3a to
3d0e027
Compare
AlexSkrypnyk
force-pushed
the
feature/fix-workflow-tests
branch
from
3d0e027 to
24bbcc4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/vortex-test-common.yml (1)
174-176: Git config guards are functional.The logic correctly sets default git user identity when absent, which prevents test failures in CI environments.
Note: A past review suggested using
-zflag instead of= ""for POSIX compliance, but the current implementation is functional.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/vortex-test-common.yml(2 hunks).vortex/tests/bats/_helper.bash(0 hunks).vortex/tests/bats/_helper.workflow.bash(0 hunks).vortex/tests/phpunit/Functional/InstallerTest.php(1 hunks)
💤 Files with no reviewable changes (2)
- .vortex/tests/bats/_helper.workflow.bash
- .vortex/tests/bats/_helper.bash
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-08T12:02:24.652Z
Learnt from: AlexSkrypnyk
Repo: drevops/vortex PR: 1896
File: .vortex/tests/bats/unit/download-db-lagoon.bats:24-25
Timestamp: 2025-08-08T12:02:24.652Z
Learning: In .vortex/tests/bats/unit Bats tests using ../_helper.bash (run_steps), prefixing a STEPS entry with "- " denotes a negative assertion (the substring must NOT appear in output). Unprefixed entries are positive assertions. Example: "- Database dump refresh requested. Will create a new dump." asserts absence; "Database dump refresh requested. Will create a new dump." asserts presence.
Applied to files:
.github/workflows/vortex-test-common.yml
🧬 Code graph analysis (1)
.vortex/tests/phpunit/Functional/InstallerTest.php (1)
.vortex/tests/phpunit/Functional/AhoyWorkflowTest.php (8)
Group(26-71)Group(73-110)Group(112-131)Group(133-198)Group(200-224)Group(226-254)Group(256-331)Group(333-406)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: build (1)
- GitHub Check: build (0)
- GitHub Check: vortex-test-workflow (3)
- GitHub Check: vortex-test-workflow (4)
- GitHub Check: vortex-test-workflow (2)
- GitHub Check: vortex-test-workflow (0)
- GitHub Check: vortex-test-workflow (1)
- GitHub Check: vortex-test-common
- GitHub Check: vortex-test-installer (8.3)
- GitHub Check: vortex-test-installer (8.2)
- GitHub Check: vortex-test-installer (8.4)
- GitHub Check: vortex-test-docs
🔇 Additional comments (3)
.github/workflows/vortex-test-common.yml (2)
178-179: Docker network stub creation is sound.The
2>/dev/null || truepattern correctly suppresses errors when the network already exists, preventing the step from failing unnecessarily.
181-181: Test group annotations verified and complete across p0-p4.All 24 tests found in the test suite are properly annotated with group markers (p0 through p4). The parallelization strategy is correctly implemented with no untagged tests requiring distribution. While distribution across groups is uneven (p0: 11, p1: 2, p2: 3, p3: 3, p4: 5), the core requirement of having all tests properly tagged for parallel batch execution is satisfied.
.vortex/tests/phpunit/Functional/InstallerTest.php (1)
32-32: Group annotation correctly applied for test parallelization.The
#[Group('p0')]attribute correctly tags this test for execution in the first matrix batch, aligning with the workflow's grouped execution strategy. The PHPUnit attribute syntax is properly used, and the required import is present on line 9.
| - name: Generate test SSH key pair used for deployment tests | ||
| run: | | ||
| mkdir -p "$HOME/.ssh" | ||
| ssh-keygen -t rsa -b 4096 -m PEM -N '' -f "$HOME/.ssh/id_rsa" | ||
| chmod 600 "$HOME/.ssh/id_rsa" | ||
| ssh-agent -a "${HOME}/ssh-agent.sock" > ssh-agent-output | ||
| export SSH_AUTH_SOCK=$(grep SSH_AUTH_SOCK ssh-agent-output | cut -d';' -f1 | cut -d= -f2) | ||
| echo "SSH_AUTH_SOCK=${SSH_AUTH_SOCK}" >> "$GITHUB_ENV" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider extracting SSH key generation to a composite action.
The SSH key generation logic at lines 144-151 is identical to lines 53-60 in the vortex-test-common job. While this duplication works, extracting it to a reusable composite action would improve maintainability and reduce the risk of the two implementations diverging.
🤖 Prompt for AI Agents
.github/workflows/vortex-test-common.yml lines 144-151: the SSH key generation
block is duplicated elsewhere (lines 53-60); extract this logic into a reusable
composite action (e.g., .github/actions/generate-ssh-key/action.yml) that runs
the mkdir/ssh-keygen/chmod/ssh-agent steps and exposes SSH_AUTH_SOCK (and any
key paths) as outputs, then replace the two inlined blocks with a single step
using: uses: ./.github/actions/generate-ssh-key and map the output into the job
environment (echo to GITHUB_ENV or set as env), ensure the action sets proper
file permissions and returns any necessary outputs so both jobs consume the same
implementation.
24bbcc4 to
d137ba2
Compare
AlexSkrypnyk
force-pushed
the
feature/fix-workflow-tests
branch
from
d137ba2 to
4fad81c
Compare
Summary by CodeRabbit
Tests
Chores