[#1560] Moved post build test to PHPUnit. #2075
Conversation
WalkthroughTest execution migrated from Bash/Bats scripts to PHPUnit framework. CircleCI post-build tests now run via PHPUnit's group filtering rather than dedicated shell scripts. CI pipeline refactored to separately install dependencies and execute grouped test suites. Changes
Sequence DiagramsequenceDiagram
participant CI as CircleCI
participant Composer as Composer
participant PHPUnit as PHPUnit
participant CircleCiAPI as CircleCI API
participant PostBuildTest as PostBuildTest
CI->>Composer: Install dependencies
Composer-->>CI: Dependencies ready
CI->>PHPUnit: phpunit --group=postbuild
PHPUnit->>PostBuildTest: setUp()
PostBuildTest->>CircleCiAPI: Get current job workflow ID
CircleCiAPI-->>PostBuildTest: workflow_id
PostBuildTest->>CircleCiAPI: Get previous job numbers
CircleCiAPI-->>PostBuildTest: [job_numbers]
loop For each previous job
PostBuildTest->>CircleCiAPI: Get job artifacts
CircleCiAPI-->>PostBuildTest: artifacts_data
PostBuildTest->>PostBuildTest: Validate coverage + Behat files
PostBuildTest->>CircleCiAPI: Get job test metadata
CircleCiAPI-->>PostBuildTest: test_metadata
PostBuildTest->>PostBuildTest: Validate test results
end
PHPUnit->>PostBuildTest: tearDown()
PHPUnit-->>CI: Test results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
AlexSkrypnyk
force-pushed
the
feature/1560-convert-postbuild-test
branch
from
4f4c7c5 to
e515794
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: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
.circleci/config.yml(1 hunks).vortex/tests/README.md(0 hunks).vortex/tests/bats/e2e/README.md(0 hunks).vortex/tests/bats/e2e/circleci.bats(0 hunks).vortex/tests/phpunit/Functional/PostBuildTest.php(1 hunks).vortex/tests/phpunit/Traits/CircleCiTrait.php(1 hunks).vortex/tests/test.postbuild.sh(0 hunks)
💤 Files with no reviewable changes (4)
- .vortex/tests/bats/e2e/README.md
- .vortex/tests/bats/e2e/circleci.bats
- .vortex/tests/README.md
- .vortex/tests/test.postbuild.sh
🧰 Additional context used
🧬 Code graph analysis (1)
.vortex/tests/phpunit/Functional/PostBuildTest.php (1)
.vortex/tests/phpunit/Traits/CircleCiTrait.php (5)
circleCiGetPreviousJobNumbers(48-77)circleCiGetJobArtifacts(88-103)circleCiExtractArtifactPaths(172-181)circleCiGetJobTestMetadata(114-129)circleCiExtractTestPaths(192-199)
⏰ 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 (3)
- 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.4)
- GitHub Check: vortex-test-installer (8.2)
- GitHub Check: vortex-test-docs
🔇 Additional comments (4)
.circleci/config.yml (1)
539-549: LGTM! Clean migration to PHPUnit.The separation of dependency installation and test execution is clear and follows best practices. The use of PHPUnit's group filtering (
--group=postbuild) provides good flexibility for test organization..vortex/tests/phpunit/Functional/PostBuildTest.php (3)
23-42: LGTM! Proper environment validation and setup.The environment checks are thorough, and the decision to skip parent setup/teardown is well-documented and appropriate since these tests don't require the full test environment setup.
55-88: Verify hard-coded parallelism assumptions.The test assumes exactly 2 parallel runners (0 and 1) and has specific expectations about which Behat features run on which runner (e.g.,
clamav.featureon runner 0 but not runner 1,search.featureon runner 1 but not runner 0).This creates tight coupling to the current CI configuration. If the parallelism level changes or features are redistributed, this test will fail.
Consider whether this level of specificity is necessary, or if the test should be more flexible. For example:
- Should the test dynamically discover the number of parallel runners?
- Should feature distribution be verified at all, or just that all expected features exist across all runners combined?
If the current approach is intentional to validate parallel job splitting, please add a comment explaining this is a deliberate smoke test of the parallelization strategy.
98-124: LGTM! Reasonable validation of test results.This test appropriately validates that expected test results are present without making assumptions about parallelism. The coupling to specific test file names is acceptable for a smoke test that verifies the CI pipeline is working as expected.
| protected function circleCiGetWorkflowIdFromJobNumber(int $jobNumber): string { | ||
| $token = getenv('TEST_CIRCLECI_TOKEN'); | ||
| $username = getenv('CIRCLE_PROJECT_USERNAME'); | ||
| $reponame = getenv('CIRCLE_PROJECT_REPONAME'); | ||
|
|
||
| $url = sprintf( | ||
| 'https://circleci.com/api/v2/project/gh/%s/%s/job/%d', | ||
| $username, | ||
| $reponame, | ||
| $jobNumber | ||
| ); | ||
|
|
||
| $response = $this->circleCiApiRequest($url, $token); | ||
| $data = json_decode($response, TRUE, 512, JSON_THROW_ON_ERROR); | ||
|
|
||
| return $data['latest_workflow']['id']; | ||
| } |
There was a problem hiding this comment.
Add validation for environment variables and response structure.
The method uses environment variables and accesses nested array keys without validation. If environment variables are empty or the API response structure changes, this will cause cryptic errors.
While PostBuildTest::setUp() validates these environment variables, the trait methods should be defensive since they could be used in other contexts. Consider adding:
protected function circleCiGetWorkflowIdFromJobNumber(int $jobNumber): string {
$token = getenv('TEST_CIRCLECI_TOKEN');
$username = getenv('CIRCLE_PROJECT_USERNAME');
$reponame = getenv('CIRCLE_PROJECT_REPONAME');
+
+ if (empty($token) || empty($username) || empty($reponame)) {
+ throw new \RuntimeException('Required CircleCI environment variables are not set');
+ }
$url = sprintf(
'https://circleci.com/api/v2/project/gh/%s/%s/job/%d',
$username,
$reponame,
$jobNumber
);
$response = $this->circleCiApiRequest($url, $token);
$data = json_decode($response, TRUE, 512, JSON_THROW_ON_ERROR);
+
+ if (!isset($data['latest_workflow']['id'])) {
+ throw new \RuntimeException('Unexpected API response structure: missing latest_workflow.id');
+ }
return $data['latest_workflow']['id'];
}📝 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.
| protected function circleCiGetWorkflowIdFromJobNumber(int $jobNumber): string { | |
| $token = getenv('TEST_CIRCLECI_TOKEN'); | |
| $username = getenv('CIRCLE_PROJECT_USERNAME'); | |
| $reponame = getenv('CIRCLE_PROJECT_REPONAME'); | |
| $url = sprintf( | |
| 'https://circleci.com/api/v2/project/gh/%s/%s/job/%d', | |
| $username, | |
| $reponame, | |
| $jobNumber | |
| ); | |
| $response = $this->circleCiApiRequest($url, $token); | |
| $data = json_decode($response, TRUE, 512, JSON_THROW_ON_ERROR); | |
| return $data['latest_workflow']['id']; | |
| } | |
| protected function circleCiGetWorkflowIdFromJobNumber(int $jobNumber): string { | |
| $token = getenv('TEST_CIRCLECI_TOKEN'); | |
| $username = getenv('CIRCLE_PROJECT_USERNAME'); | |
| $reponame = getenv('CIRCLE_PROJECT_REPONAME'); | |
| if (empty($token) || empty($username) || empty($reponame)) { | |
| throw new \RuntimeException('Required CircleCI environment variables are not set'); | |
| } | |
| $url = sprintf( | |
| 'https://circleci.com/api/v2/project/gh/%s/%s/job/%d', | |
| $username, | |
| $reponame, | |
| $jobNumber | |
| ); | |
| $response = $this->circleCiApiRequest($url, $token); | |
| $data = json_decode($response, TRUE, 512, JSON_THROW_ON_ERROR); | |
| if (!isset($data['latest_workflow']['id'])) { | |
| throw new \RuntimeException('Unexpected API response structure: missing latest_workflow.id'); | |
| } | |
| return $data['latest_workflow']['id']; | |
| } |
🤖 Prompt for AI Agents
.vortex/tests/phpunit/Traits/CircleCiTrait.php lines 21-37: the method assumes
environment variables and nested response keys always exist; validate
TEST_CIRCLECI_TOKEN, CIRCLE_PROJECT_USERNAME and CIRCLE_PROJECT_REPONAME are
non-empty and throw a clear InvalidArgumentException if any are missing, ensure
circleCiApiRequest returned a non-empty string and wrap json_decode in try/catch
to surface JSON errors, then verify the decoded array contains 'latest_workflow'
as an array and an 'id' key (non-empty string) before returning; if keys are
missing or types are unexpected, throw a RuntimeException with the response body
or decoded structure included for debugging.
| protected function circleCiGetPreviousJobNumbers(int $currentJobNumber): array { | ||
| $token = getenv('TEST_CIRCLECI_TOKEN'); | ||
| $workflowId = $this->circleCiGetWorkflowIdFromJobNumber($currentJobNumber); | ||
|
|
||
| $url = sprintf('https://circleci.com/api/v2/workflow/%s/job', $workflowId); | ||
| $response = $this->circleCiApiRequest($url, $token); | ||
| $workflowData = json_decode($response, TRUE, 512, JSON_THROW_ON_ERROR); | ||
|
|
||
| // Find the current job and get its dependencies. | ||
| $dependenciesJobIds = []; | ||
| foreach ($workflowData['items'] as $item) { | ||
| if ($item['job_number'] == $currentJobNumber) { | ||
| $dependenciesJobIds = $item['dependencies'] ?? []; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| // Map dependency IDs to job numbers. | ||
| $previousJobNumbers = []; | ||
| foreach ($dependenciesJobIds as $dependencyId) { | ||
| foreach ($workflowData['items'] as $item) { | ||
| if ($item['id'] === $dependencyId) { | ||
| $previousJobNumbers[] = $item['job_number']; | ||
| break; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return $previousJobNumbers; | ||
| } |
There was a problem hiding this comment.
Use strict comparison and add response validation.
Two issues to address:
-
Line 59: Uses loose comparison (
==) instead of strict comparison (===). This could cause type coercion issues if job numbers are compared as strings vs integers. -
Missing validation: The method doesn't validate that required keys exist in the API response before accessing them.
Apply these fixes:
protected function circleCiGetPreviousJobNumbers(int $currentJobNumber): array {
$token = getenv('TEST_CIRCLECI_TOKEN');
$workflowId = $this->circleCiGetWorkflowIdFromJobNumber($currentJobNumber);
$url = sprintf('https://circleci.com/api/v2/workflow/%s/job', $workflowId);
$response = $this->circleCiApiRequest($url, $token);
$workflowData = json_decode($response, TRUE, 512, JSON_THROW_ON_ERROR);
+
+ if (!isset($workflowData['items'])) {
+ throw new \RuntimeException('Unexpected API response: missing items');
+ }
// Find the current job and get its dependencies.
$dependenciesJobIds = [];
foreach ($workflowData['items'] as $item) {
- if ($item['job_number'] == $currentJobNumber) {
+ if ($item['job_number'] === $currentJobNumber) {
$dependenciesJobIds = $item['dependencies'] ?? [];
break;
}
}
// Map dependency IDs to job numbers.
$previousJobNumbers = [];
foreach ($dependenciesJobIds as $dependencyId) {
foreach ($workflowData['items'] as $item) {
if ($item['id'] === $dependencyId) {
$previousJobNumbers[] = $item['job_number'];
break;
}
}
}
return $previousJobNumbers;
}🤖 Prompt for AI Agents
.vortex/tests/phpunit/Traits/CircleCiTrait.php around lines 48-77: the method
should use strict comparison and validate the API response shape before
accessing keys; change the loose == to a strict === (ensuring types match, e.g.,
cast $item['job_number'] to int or cast $currentJobNumber to string
consistently), and add validation after json_decode to confirm the result is an
array with an 'items' key that is an array; for each item check that
'job_number' and 'id' exist before using them (skip or throw a descriptive
exception if the response is missing expected fields) and return an empty array
or fail fast if validation fails.
| protected function circleCiApiRequest(string $url, string $token): string { | ||
| $ch = curl_init(); | ||
| curl_setopt($ch, CURLOPT_URL, $url); | ||
| curl_setopt($ch, CURLOPT_RETURNTRANSFER, TRUE); | ||
| curl_setopt($ch, CURLOPT_HTTPHEADER, [ | ||
| 'Circle-Token: ' . $token, | ||
| ]); | ||
|
|
||
| $response = curl_exec($ch); | ||
| $httpCode = curl_getinfo($ch, CURLINFO_HTTP_CODE); | ||
| curl_close($ch); | ||
|
|
||
| if ($httpCode !== 200) { | ||
| throw new \RuntimeException(sprintf('CircleCI API request failed with HTTP code %d: %s', $httpCode, $response)); | ||
| } | ||
|
|
||
| return $response; | ||
| } |
There was a problem hiding this comment.
Add error handling for curl_exec failure.
curl_exec() returns false on failure, but this isn't checked. If a network error or curl failure occurs, false would be passed as the response, potentially causing issues in downstream code.
Apply this fix:
protected function circleCiApiRequest(string $url, string $token): string {
$ch = curl_init();
curl_setopt($ch, CURLOPT_URL, $url);
curl_setopt($ch, CURLOPT_RETURNTRANSFER, TRUE);
curl_setopt($ch, CURLOPT_HTTPHEADER, [
'Circle-Token: ' . $token,
]);
$response = curl_exec($ch);
+ if ($response === FALSE) {
+ $error = curl_error($ch);
+ curl_close($ch);
+ throw new \RuntimeException(sprintf('cURL request failed: %s', $error));
+ }
+
$httpCode = curl_getinfo($ch, CURLINFO_HTTP_CODE);
curl_close($ch);
if ($httpCode !== 200) {
throw new \RuntimeException(sprintf('CircleCI API request failed with HTTP code %d: %s', $httpCode, $response));
}
return $response;
}🤖 Prompt for AI Agents
.vortex/tests/phpunit/Traits/CircleCiTrait.php around lines 142-159: curl_exec()
return value is not checked, so failures produce false as $response and lose
error info; after curl_exec($ch) check if $response === false, capture
curl_error($ch) and curl_errno($ch), close the handle, and throw a
RuntimeException including the curl error/errno and the URL/token context;
otherwise continue to get the HTTP code and if it’s not 200 throw the existing
runtime exception including the actual $response.
| protected function circleCiExtractArtifactPaths(array $artifactsData, int $nodeIndex): array { | ||
| $paths = []; | ||
| foreach ($artifactsData['items'] as $item) { | ||
| if ($item['node_index'] === $nodeIndex) { | ||
| $paths[] = $item['path']; | ||
| } | ||
| } | ||
|
|
||
| return $paths; | ||
| } |
There was a problem hiding this comment.
Add validation for response structure.
The method accesses $artifactsData['items'] without checking if the key exists. If the API response structure changes or is malformed, this will cause an error.
Apply this fix:
protected function circleCiExtractArtifactPaths(array $artifactsData, int $nodeIndex): array {
$paths = [];
+ if (!isset($artifactsData['items'])) {
+ return $paths;
+ }
+
foreach ($artifactsData['items'] as $item) {
if ($item['node_index'] === $nodeIndex) {
$paths[] = $item['path'];
}
}
return $paths;
}📝 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.
| protected function circleCiExtractArtifactPaths(array $artifactsData, int $nodeIndex): array { | |
| $paths = []; | |
| foreach ($artifactsData['items'] as $item) { | |
| if ($item['node_index'] === $nodeIndex) { | |
| $paths[] = $item['path']; | |
| } | |
| } | |
| return $paths; | |
| } | |
| protected function circleCiExtractArtifactPaths(array $artifactsData, int $nodeIndex): array { | |
| $paths = []; | |
| if (!isset($artifactsData['items'])) { | |
| return $paths; | |
| } | |
| foreach ($artifactsData['items'] as $item) { | |
| if ($item['node_index'] === $nodeIndex) { | |
| $paths[] = $item['path']; | |
| } | |
| } | |
| return $paths; | |
| } |
🤖 Prompt for AI Agents
.vortex/tests/phpunit/Traits/CircleCiTrait.php around lines 172-181: the method
assumes $artifactsData['items'] exists and is an array and that each item has
'node_index' and 'path'; add defensive validation: if 'items' is missing or not
an array return empty array immediately, and inside the loop ensure each $item
is an array and has both 'node_index' and 'path' keys before accessing them (use
strict comparisons for node_index match); this prevents undefined index errors
on malformed API responses.
| protected function circleCiExtractTestPaths(array $testsData): array { | ||
| $paths = []; | ||
| foreach ($testsData['items'] as $item) { | ||
| $paths[] = $item['file']; | ||
| } | ||
|
|
||
| return $paths; | ||
| } |
There was a problem hiding this comment.
Add validation for response structure.
Similar to circleCiExtractArtifactPaths, this method should validate that the items key exists before accessing it.
Apply this fix:
protected function circleCiExtractTestPaths(array $testsData): array {
$paths = [];
+ if (!isset($testsData['items'])) {
+ return $paths;
+ }
+
foreach ($testsData['items'] as $item) {
$paths[] = $item['file'];
}
return $paths;
}📝 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.
| protected function circleCiExtractTestPaths(array $testsData): array { | |
| $paths = []; | |
| foreach ($testsData['items'] as $item) { | |
| $paths[] = $item['file']; | |
| } | |
| return $paths; | |
| } | |
| protected function circleCiExtractTestPaths(array $testsData): array { | |
| $paths = []; | |
| if (!isset($testsData['items'])) { | |
| return $paths; | |
| } | |
| foreach ($testsData['items'] as $item) { | |
| $paths[] = $item['file']; | |
| } | |
| return $paths; | |
| } |
🤖 Prompt for AI Agents
.vortex/tests/phpunit/Traits/CircleCiTrait.php around lines 192-199: the method
unconditionally accesses $testsData['items']; validate that 'items' exists and
is an array before iterating, and guard each item has a 'file' key before
pushing to $paths; if validation fails, return an empty array (or handle
consistently with circleCiExtractArtifactPaths), so replace the direct loop with
a conditional check like isset($testsData['items']) &&
is_array($testsData['items']) then loop and only add $item['file'] when
isset($item['file']).
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #2075 +/- ##
===========================================
- Coverage 54.74% 54.58% -0.16%
===========================================
Files 92 92
Lines 5692 5692
Branches 44 44
===========================================
- Hits 3116 3107 -9
- Misses 2576 2585 +9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Closes #
Summary by CodeRabbit
Tests
Chores