chore(ci): add Windows compatibility testing#13049
Conversation
Run isolated tests on a native Windows runner to detect filesystem, path, filename, and NTFS case-sensitivity problems not covered by Linux CI. Use PHP 8.2 so the job tests OpenEMR's minimum supported version without duplicating the existing PHP version matrix. Assisted-by: ChatGPT
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Windows GitHub Actions workflow for path validation, PHPUnit execution, JUnit reporting, and workspace checks; updates FHIR export cleanup and group UUID validation; and aligns PHPStan baseline entries. ChangesWindows compatibility workflow
FHIR export safeguards
PHPStan baseline updates
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant GitHubActions
participant WindowsRunner
participant PHPUnit
participant ArtifactStorage
GitHubActions->>WindowsRunner: checkout and configure PHP 8.2
WindowsRunner->>WindowsRunner: validate tracked paths
WindowsRunner->>PHPUnit: run isolated suite
PHPUnit-->>WindowsRunner: write JUnit XML
WindowsRunner->>ArtifactStorage: upload JUnit artifact
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/windows-tests.yml:
- Line 79: Disable Git path quoting in both path-list retrieval sites: update
the ls-files invocation assigned to $paths and the ls-files invocation iterated
by the foreach loop in .github/workflows/windows-tests.yml at lines 60-60 and
79-79 to use core.quotePath=false, preserving raw readable paths for grouping,
logging, and validation.
- Around line 38-41: Update the actions/checkout step in the Windows test
workflow to set persist-credentials to false alongside fetch-depth, preventing
the GitHub token from being stored in the local Git configuration.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 464706a8-eee2-419e-8ea3-3598f18bf904
📒 Files selected for processing (1)
.github/workflows/windows-tests.yml
| jobs: | ||
| windows-compatibility: | ||
| name: Windows compatibility | ||
| runs-on: windows-2025 |
There was a problem hiding this comment.
as the kids say…
"Let's goooooo!"
There was a problem hiding this comment.
yep I need some hand holding in this space
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #13049 +/- ##
============================================
- Coverage 27.47% 25.58% -1.89%
- Complexity 85472 85478 +6
============================================
Files 3943 3943
Lines 422409 422167 -242
============================================
- Hits 116041 108022 -8019
- Misses 306368 314145 +7777
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
I'm looking into some other gotcha area concerning windows I've run across over the year. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/windows-tests.yml:
- Around line 107-111: Remove the permissive if conditions from the Windows
workflow steps at .github/workflows/windows-tests.yml lines 107-111, 131, and
140. Delete always() from the “Report isolated-suite Windows baseline” step and
!cancelled() from the artifact-warning and checkout-related steps, allowing the
default success() condition to prevent them from running after setup or
validation failures while preserving normal execution after the
continue-on-error test step.
- Line 131: Remove the explicit if condition from the artifact upload step in
the Windows workflow. Let the default success() condition govern execution so
uploads still occur after the continue-on-error windows-tests step fails, but
are skipped when earlier workflow steps abort before tests run.
- Line 140: Remove the explicit if condition from the cleanup check in the
Windows workflow, allowing the default success() condition to control execution.
Preserve the cleanup step and its behavior, including execution after
continue-on-error test failures while skipping it when an earlier validation
step fails.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c1a745be-4aca-4d91-a4df-e9b3245dab14
📒 Files selected for processing (1)
.github/workflows/windows-tests.yml
| retention-days: 14 | ||
|
|
||
| - name: Verify tests left the checkout clean | ||
| if: ${{ !cancelled() }} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Remove if: ${{ !cancelled() }} to prevent evaluating an invalid checkout state on early workflow failures.
For the same reason as the artifact upload step, the default success() condition is sufficient to run this cleanup check even if the continue-on-error test suite fails. Using !cancelled() inappropriately executes this check if an early validation step fails, potentially evaluating an incomplete or broken checkout state.
♻️ Proposed fix
- name: Verify tests left the checkout clean
- if: ${{ !cancelled() }}
shell: pwsh📝 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: ${{ !cancelled() }} |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/windows-tests.yml at line 140, Remove the explicit if
condition from the cleanup check in the Windows workflow, allowing the default
success() condition to control execution. Preserve the cleanup step and its
behavior, including execution after continue-on-error test failures while
skipping it when an earlier validation step fails.
…e skipped. Non-blocking PHPUnit failure → summary and JUnit artifact still run. Successful PHPUnit run → reports passed. Clean-checkout verification runs only when the preceding required steps succeeded. coderabbitai review fixes. he makes it too easy!
… group validation This may help some tests failure rate. - Initialize \$exportWriter to null before try block to prevent undefined variable errors in finally block - Add null check in finally block before accessing \$exportWriter methods - Add early validation in getPatientUuidsForGroup() to fail fast on empty group IDs - Fixes timeout issue in GroupExportFhirApiTest::testGroupExportWithNonExistingGroupId - Improves error handling and prevents resource exhaustion from hanging finally blocks"
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/RestControllers/FHIR/Operations/FhirOperationExportRestController.php`:
- Line 738: Update the UUID filter in the export flow around
UtilsService::getUuidFromReference() so it explicitly rejects null and only
appends non-empty UUID strings to $patientUuids. Preserve the existing
collection behavior for valid extracted UUIDs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6e0dfa75-40d2-43bf-b8e8-ca364d3c8ef8
📒 Files selected for processing (1)
src/RestControllers/FHIR/Operations/FhirOperationExportRestController.php
|
@bradymiller @kojiromike I'm done if you want to look over. |
|
okay @bradymiller I've dabbled in testing now so can I get back to being a developer and leave tests to my intern, Claude and Goober(ChatGPT and I think it takes offence to the name!)? lol |
kojiromike
left a comment
kojiromike
left a comment
There was a problem hiding this comment.
Thanks for adding Windows compatibility coverage — the workflow is well put together (concurrency block, least-privilege permissions, persist-credentials: false, XDEBUG_MODE: off, and the case-collision / reserved-name detectors are a nice portable safety net). The core FHIR fix (initializing $exportWriter and guarding the finally) is correct too.
A couple of code issues to fix first — see coderabbit's on the $uuid check (a null regression) and mine on the empty($groupId) guard.
On the .phpstan/baseline/ changes: they're tangled (a dropped forbiddenEval include plus unrelated Tree.class.php drift). Rather than hand-edit, please fix the code issues above, then regenerate the baseline from scratch (composer phpstan-baseline-reset) so the diff reflects only what this PR actually changes.
| - name: Setup PHP 8.2 and Composer | ||
| uses: ./.github/actions/setup-php-composer | ||
| with: | ||
| # Linux CI owns PHP-version coverage; Windows is the variable here. |
There was a problem hiding this comment.
Nit: this comment is indented 10 spaces while php-version: below it is at 8 — valid YAML but visually misaligned. Align the comment with the keys it documents.
| # Linux CI owns PHP-version coverage; Windows is the variable here. | |
| # Linux CI owns PHP-version coverage; Windows is the variable here. |
There was a problem hiding this comment.
baseline came from ci generated baseline in test in last commit. I went through at least four conflicts from other merged PRs. I think all were phpstan issues.
I put together a new PR commit process using pre-commits and emulating upstream more closely so hopefully next PR I will test and get it figured out.
| @@ -729,7 +735,7 @@ private function getPatientUuidsForGroup($groupId) | |||
| foreach ($group->getMember() as $member) { | |||
| if (!empty($member->getEntity()) && !empty($member->getEntity()->getReference())) { | |||
| $uuid = UtilsService::getUuidFromReference($member->getEntity()); | |||
There was a problem hiding this comment.
Coderabbit's comment below is right, but I'd fix it at the source…
| $uuid = UtilsService::getUuidFromReference($member->getEntity()); | |
| $uuid = UtilsService::getUuidFromReference($member->getEntity()) ?? ''; |
| if (empty($groupId)) { | ||
| throw new \InvalidArgumentException("Group ID cannot be empty"); | ||
| } |
There was a problem hiding this comment.
The commit message says this fixes testGroupExportWithNonExistingGroupId, but a non-existing group ID is a well-formed, non-empty value — it passes this guard and proceeds to getOne(), which returns no data and yields []. So this only guards genuinely empty input, not the non-existing case described. Also worth confirming the caller catches InvalidArgumentException, otherwise this surfaces as a 500 rather than a clean OperationOutcome.
Also, please avoid empty(). It's an old and broken php function that we shouldn't use anymore.
| if (empty($groupId)) { | |
| throw new \InvalidArgumentException("Group ID cannot be empty"); | |
| } | |
| if ($groupId === null || $groupId === '') { | |
| throw new \InvalidArgumentException("Group ID cannot be empty"); | |
| } |
There was a problem hiding this comment.
I was going to fix coderabbit last review this morning because it got to late for me last night. Thanks for reminder and review.
|
Looks like Bulk EHI is broken. I have to setup debug mode to track. Haven't test on easy dev yet. I haven't touched any of this modules source. If anyone wants to test on a release demo that be helpful because I have to finish the export FHIR endpoint which I have touched that code in this PR. 
Half done. Still need to fetch the report. 
|
Make UtilsService::getUuidFromReference() null-tolerant (accept ?FHIRReference and return null for a null reference), since FHIR getters can return null despite non-null @var annotations on the generated R4 classes. Group-member entity resolution then drops its now-redundant null guards and relies on the utility plus the existing empty-UUID filter. Replace ambiguous empty() checks with explicit typed comparisons, reject null or empty group IDs, and tighten resource collection types and comparisons. Align the OpenAPI named arguments with their declared parameter order. Verified that processExport() catches InvalidArgumentException and returns a 400 Bad Request OperationOutcome, so the group ID guard does not introduce 400-to-500 response drift. A well-formed but nonexistent group ID continues through the existing no-members path. Assisted-by: ChatGPT Work Assisted-by: Claude
|
We've been all over this workflow and believe we've narrowed in but will prove tomorrow. I'll just have to use docker. Tried hard to avoid but if I can prove then we can communicate to others in contrib. Status: PHPStan baseline on native Windows The PR's actual code change is clean — phpstan-analyze-staged consistently reports a single expected change: the 'system/' . mixed count in binaryOp.invalid.php drops from 2 → 1 (one usage removed in FhirOperationExportRestController). That one-line baseline decrement is applied. The gotcha we hit: running the documented composer phpstan-baseline full regen on a native Windows host produces a divergent baseline — a wall of added entries under the custom openemr.noGlobalNsFunctions and openemr.forbidDirectSessionWrite rules (~560+) that CI's Linux baseline doesn't contain. Committing that regen would fail the git diff --exit-code .phpstan/baseline/ guard. Two contributing factors identified: (1) interrupted regens (local OOM) left orphaned, unreferenced baseline files on disk that made every already-baselined error report as "new," and (2) even on a clean tree, the full regen still diverges — pointing at Windows path/namespace resolution in those two custom rules. Resolution used: surgical hand-edit of the single real baseline change, letting CI (Linux, correct PHP matrix) validate. |
… entries Assisted-By: Claude
|
Need a comment in contrib maybe Need input! I'm going to publish ot forum and elsewhere a how to for alternative to xampp. Two option of WSL2 or Docker for Windows. btw: I need put up PR for issue #13054 and will use docker going forward. I had to try to make windows work so thanks for patience. |
2 participants
Run isolated tests on a native Windows runner to detect filesystem, path, filename, and NTFS case-sensitivity problems not covered by Linux CI. Plans are to add to this.
Use PHP 8.2 so the job tests OpenEMR's minimum supported version without duplicating the existing PHP version matrix.
Assisted-by: ChatGPT Code
Changes proposed in this pull request:
add workflow
Was an AI assistant used? Yes