Fixed coding standards in workflow and installer tests.#2139
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughRefactors Rector configs to skip NewlineBetweenClassLikeStmtsRector; renames many test variables and CircleCiTrait parameters to snake_case; hardens CircleCI API methods with environment checks, JSON/structure validation, and explicit errors; adds a CI "Lint code" step running phpcs/phpstan/rector in the tests directory. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Test as PostBuildTest
participant Trait as CircleCiTrait
participant API as CircleCI API
Test->>Trait: circleCiGetWorkflowIdFromJobNumber(job_number)
Trait->>Trait: validate env (token, project info)
Trait->>API: GET /project/.../job/{job_number} (Authorization)
API-->>Trait: 200 + JSON
Trait->>Trait: decode JSON, validate shape, extract workflow_id
Trait-->>Test: return workflow_id
alt fetch previous jobs
Test->>Trait: circleCiGetPreviousJobNumbers(current_job_number)
Trait->>API: GET /workflow/{workflow_id}/job
API-->>Trait: 200 + JSON (jobs, dependencies)
Trait->>Trait: validate, map dependency ids -> job_numbers
Trait-->>Test: return previous_job_numbers
end
alt fetch artifacts & tests
Test->>Trait: circleCiGetJobArtifacts(job_number)
Trait->>API: GET /job/{job_number}/artifacts
API-->>Trait: 200 + JSON (artifacts list)
Trait->>Trait: validate artifacts_data, extract artifact_paths
Trait-->>Test: return artifact_paths
Test->>Trait: circleCiGetJobTestMetadata(job_number)
Trait->>API: GET /job/{job_number}/tests
API-->>Trait: 200 + JSON (tests list)
Trait->>Trait: validate tests_data, extract test_paths
Trait-->>Test: return test_paths
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (2)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (2)
.vortex/installer/composer.lockis excluded by!**/*.lock.vortex/tests/composer.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
.vortex/installer/rector.php(2 hunks).vortex/tests/phpunit/Functional/PostBuildTest.php(2 hunks).vortex/tests/phpunit/Traits/CircleCiTrait.php(4 hunks).vortex/tests/rector.php(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-01T08:10:15.903Z
Learnt from: AlexSkrypnyk
Repo: drevops/vortex PR: 1693
File: .vortex/installer/src/Prompts/Handlers/Internal.php:52-52
Timestamp: 2025-06-01T08:10:15.903Z
Learning: The File utility class in .vortex/installer/src/Utils/File.php extends AlexSkrypnyk\File\File, and methods like collapseRepeatedEmptyLines() are available from this external parent class dependency.
Applied to files:
.vortex/tests/rector.php
🧬 Code graph analysis (1)
.vortex/tests/phpunit/Functional/PostBuildTest.php (1)
.vortex/tests/phpunit/Traits/CircleCiTrait.php (5)
circleCiGetPreviousJobNumbers(60-114)circleCiGetJobArtifacts(125-150)circleCiExtractArtifactPaths(237-255)circleCiGetJobTestMetadata(161-186)circleCiExtractTestPaths(266-286)
⏰ 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). (11)
- GitHub Check: build (1)
- GitHub Check: build (0)
- GitHub Check: vortex-test-installer (8.3)
- GitHub Check: vortex-test-installer (8.4)
- GitHub Check: vortex-test-workflow (3)
- GitHub Check: vortex-test-workflow (2)
- GitHub Check: vortex-test-workflow (1)
- GitHub Check: vortex-test-workflow (0)
- GitHub Check: vortex-test-common
- GitHub Check: vortex-test-workflow (4)
- GitHub Check: vortex-test-docs
🔇 Additional comments (10)
.vortex/tests/rector.php (2)
17-17: LGTM!The import is correctly added and properly ordered among other CodingStyle imports.
54-54: LGTM!The skip rule is correctly added in alphabetical order, aligning with project coding standards.
.vortex/installer/rector.php (1)
19-19: LGTM!The import and skip rule additions mirror the changes in
.vortex/tests/rector.php, maintaining consistent Rector configuration across test and installer directories.Also applies to: 60-60
.vortex/tests/phpunit/Functional/PostBuildTest.php (1)
56-57: LGTM!All other variable renames correctly follow snake_case conventions and align with the updated CircleCiTrait method signatures.
Also applies to: 62-62, 65-66, 77-78, 99-100, 105-107
.vortex/tests/phpunit/Traits/CircleCiTrait.php (6)
60-114: LGTM!Excellent improvements to input validation and error handling. The method now:
- Validates required environment variables
- Checks API response structure at multiple levels
- Handles malformed data gracefully
- Provides clear error messages
The refactoring significantly improves robustness.
125-150: LGTM!The parameter rename and added validation follow the same robust pattern as other methods in this trait.
161-186: LGTM!Consistent validation pattern applied, improving method robustness.
199-224: LGTM!Excellent improvements to error handling:
- Input validation for URL and token
- HTTP status code checking
- Explicit handling of curl failures
- Clear error messages with context
These changes make API interactions much more reliable and debuggable.
237-255: LGTM!The parameter renames and added validation make this method more robust:
- Early return for invalid data
- Per-item validation before property access
- Strict equality check for node_index
266-286: LGTM!Excellent data validation:
- Early return for invalid structure
- Per-item array validation
- Explicit string type check for file field
This prevents type errors and ensures the method returns clean data.
| if (empty($data['latest_workflow'] ?? []) || (is_array($data['latest_workflow']) && empty($data['latest_workflow']['id'] ?? ''))) { | ||
| throw new \RuntimeException('Unable to retrieve workflow ID from CircleCI API response.'); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider simplifying the validation logic.
The current validation works but is more complex than necessary. The condition checks both if latest_workflow is empty and if the id within it is empty, which can be simplified.
Apply this diff to simplify:
- if (empty($data['latest_workflow'] ?? []) || (is_array($data['latest_workflow']) && empty($data['latest_workflow']['id'] ?? ''))) {
+ if (empty($data['latest_workflow']['id'] ?? '')) {
throw new \RuntimeException('Unable to retrieve workflow ID from CircleCI API response.');
}This simplified version checks the same condition: whether the workflow ID exists and is non-empty.
📝 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.
| if (empty($data['latest_workflow'] ?? []) || (is_array($data['latest_workflow']) && empty($data['latest_workflow']['id'] ?? ''))) { | |
| throw new \RuntimeException('Unable to retrieve workflow ID from CircleCI API response.'); | |
| } | |
| if (empty($data['latest_workflow']['id'] ?? '')) { | |
| throw new \RuntimeException('Unable to retrieve workflow ID from CircleCI API response.'); | |
| } |
🤖 Prompt for AI Agents
.vortex/tests/phpunit/Traits/CircleCiTrait.php around lines 44 to 46: the
validation is overly verbose; replace the two-part condition with a single check
that the workflow ID exists and is non-empty by testing
empty($data['latest_workflow']['id'] ?? ''), and throw the same RuntimeException
when that check fails.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #2139 +/- ##
========================================
Coverage 70.89% 70.89%
========================================
Files 99 99
Lines 5075 5075
Branches 44 44
========================================
Hits 3598 3598
Misses 1477 1477 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
AlexSkrypnyk
force-pushed
the
feature/workflow-tests-udpate-cs
branch
from
17f82b9 to
f68f0b7
Compare
AlexSkrypnyk
force-pushed
the
feature/workflow-tests-udpate-cs
branch
from
f68f0b7 to
a4c1121
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/vortex-test-common.yml(1 hunks)
⏰ 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). (5)
- GitHub Check: build (0)
- GitHub Check: build (1)
- GitHub Check: vortex-test-common
- GitHub Check: vortex-test-installer (8.4)
- GitHub Check: vortex-test-installer (8.3)
| - name: Lint code | ||
| run: php -d memory_limit=-1 $(which composer) lint | ||
| working-directory: .vortex/tests | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Simplify the Composer command invocation.
The new lint step is a good addition that aligns with the PR's coding standards objectives. However, line 163 uses $(which composer) when just composer is more idiomatic, since Composer should already be in the PATH after the install step on line 159.
Apply this diff to simplify the command:
- run: php -d memory_limit=-1 $(which composer) lint
+ run: php -d memory_limit=-1 composer lint📝 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.
| - name: Lint code | |
| run: php -d memory_limit=-1 $(which composer) lint | |
| working-directory: .vortex/tests | |
| - name: Lint code | |
| run: php -d memory_limit=-1 composer lint | |
| working-directory: .vortex/tests |
🤖 Prompt for AI Agents
.github/workflows/vortex-test-common.yml around lines 162 to 165: the lint step
currently invokes Composer via $(which composer); replace that with the simpler,
idiomatic composer invocation so the run line becomes "php -d memory_limit=-1
composer lint" (keeping working-directory .vortex/tests unchanged).
a4c1121 to
8e2c6ee
Compare
AlexSkrypnyk
force-pushed
the
feature/workflow-tests-udpate-cs
branch
from
8e2c6ee to
f2a4975
Compare
1 participant
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.