Skip to content

Revert "Enable windows tests on PRs"#15564

Closed
calixtus wants to merge 1 commit intomainfrom
revert-15530-subhramit-ci
Closed

Revert "Enable windows tests on PRs"#15564
calixtus wants to merge 1 commit intomainfrom
revert-15530-subhramit-ci

Conversation

@calixtus
Copy link
Copy Markdown
Member

Reverts #15530

Without having the issues fixed, causes heavy confusion by contributors.

@qodo-free-for-open-source-projects
Copy link
Copy Markdown
Contributor

Review Summary by Qodo

Restrict Windows tests to Gradle Wrapper PRs only

🐞 Bug fix

Grey Divider

Walkthroughs

Description
• Restrict Windows tests to Gradle Wrapper updates only
• Prevent unnecessary test runs on all pull requests
• Revert overly broad CI condition that caused confusion
Diagram
flowchart LR
  A["PR trigger condition"] -->|"Previously: all PRs"| B["Run Windows tests"]
  A -->|"Now: Gradle Wrapper PRs only"| C["Selective Windows tests"]
Loading

Grey Divider

File Changes

1. .github/workflows/tests-code.yml ⚙️ Configuration changes +1/-1

Restrict Windows tests to Gradle Wrapper updates

• Modified the tests-windows job condition to only run on pull requests with titles starting with
 'Update Gradle Wrapper'
• Changed from running on all pull request events to a filtered condition
• Prevents unnecessary Windows test execution on unrelated PRs

.github/workflows/tests-code.yml


Grey Divider

Qodo Logo

@qodo-free-for-open-source-projects
Copy link
Copy Markdown
Contributor

qodo-free-for-open-source-projects bot commented Apr 16, 2026

Code Review by Qodo

🐞 Bugs (1)   📘 Rule violations (1)   📎 Requirement gaps (0)
🐞\ ☼ Reliability (1)
📘\ ⚙ Maintainability (1)

Grey Divider


Remediation recommended

1. Hard-coded Update Gradle Wrapper 📘
Description
The workflow change introduces a hard-coded PR-title prefix string (Update Gradle Wrapper) to
control CI behavior, which is a brittle magic string and can drift over time. This violates the
guideline to avoid hard-coded identifiers/magic strings and prefer shared/derived context where
possible.
Code

.github/workflows/tests-code.yml[285]

+        (github.event_name == 'pull_request' && startsWith(github.event.pull_request.title, 'Update Gradle Wrapper'))
Evidence
PR Compliance ID 35 requires avoiding hard-coded upstream identifiers and magic strings; the
modified condition uses a new inline string literal in workflow logic (`startsWith(..., 'Update
Gradle Wrapper')`).

.github/workflows/tests-code.yml[285-285]
Best Practice: Learned patterns

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A hard-coded PR-title prefix (`Update Gradle Wrapper`) is used to gate the Windows job, creating a brittle magic string.

## Issue Context
The compliance guideline asks to avoid hard-coded identifiers/magic strings in workflows when possible.

## Fix Focus Areas
- .github/workflows/tests-code.yml[285-285]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Title-gated Windows tests 🐞
Description
The tests-windows job now runs on pull_request only when the PR title starts with `Update Gradle
Wrapper`, so Windows CI can be silently skipped for PRs that need Windows coverage (including manual
Gradle wrapper updates) if the title doesn’t match. This makes CI behavior depend on editable
metadata instead of the actual diff, increasing the chance of merging Windows-breaking changes
without a Windows signal.
Code

.github/workflows/tests-code.yml[285]

+        (github.event_name == 'pull_request' && startsWith(github.event.pull_request.title, 'Update Gradle Wrapper'))
Evidence
The PR changes the PR condition for the Windows unit test job to depend on
startsWith(github.event.pull_request.title, ...). In the same workflow file there is already an
example of gating work based on changed files (tj-actions/changed-files), illustrating an approach
tied to actual code changes rather than title metadata.

.github/workflows/tests-code.yml[278-286]
.github/workflows/tests-code.yml[303-326]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The Windows unit test job (`tests-windows`) is currently gated on PR title prefix (`startsWith(github.event.pull_request.title, ...)`). This can cause Windows tests to be skipped even when relevant files change, solely because the PR title doesn’t match the expected prefix.

### Issue Context
This workflow already contains a pattern for change-based gating (`tj-actions/changed-files`) in `tests-windows-file-based`, which ties behavior to the diff rather than metadata.

### Fix Focus Areas
- .github/workflows/tests-code.yml[278-286]
- .github/workflows/tests-code.yml[303-326]

### Suggested fix
Replace the PR-title condition with a change-based (or label-based) condition. For example:
- Create a small, cheap “detect changes” job on Linux that uses `tj-actions/changed-files` to detect Gradle wrapper/build-related changes and exposes an output.
- Make `tests-windows` depend on that job and set `if:` to run on PRs only when the output indicates relevant files changed.

This preserves the intent (avoid running Windows tests on every PR) while ensuring Windows tests run when the code changes warrant it.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

ⓘ The new review experience is currently in Beta. Learn more

Grey Divider

Qodo Logo

@calixtus calixtus closed this Apr 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant