Repository navigation
Run build and packaging checks on pull requests and main - #138
Conversation
CatarinaGamboa
left a comment
CatarinaGamboa
left a comment
There was a problem hiding this comment.
Two things in the new workflow, both about the checks not quite guarding what they're meant to.
Reviewed with Claude Code (reviewer + adversarial agents per PR, findings checked against the code before posting).
| jobs: | ||
| checks: | ||
| name: Checks | ||
| if: github.event_name == 'push' || github.event.pull_request.head.repo.full_name != github.repository |
There was a problem hiding this comment.
A skipped Checks job counts as passing. For PRs from branches in this repo, the pull_request run still creates a Checks job, which this if skips. GitHub reports skipped jobs as successful, and a skipped job "will not prevent a pull request from merging, even if it is a required check" (docs). This PR's head commit shows it: one Checks from the push run (passed) and one from the PR run (skipped).
Once #126 makes Checks required on main, a PR could merge while its push run is failing or still running. The simplest fix is to drop the if and accept one duplicate run per PR push. Otherwise, make sure the required check can only be satisfied by a job that actually ran.
There was a problem hiding this comment.
Removed the job condition in 3043e40, so the required Checks job executes on both push and PR runs rather than succeeding through a skip.
| - name: Setup Java | ||
| uses: actions/setup-java@v4 | ||
| with: | ||
| java-version: 21 |
There was a problem hiding this comment.
CI builds the server with JDK 21, but publish.yml builds the shipped JAR with JDK 20. server/pom.xml sets <source>20</source> / <target>20</target> but no <release>, so under JDK 21 javac compiles against the JDK 21 class library and accepts 21-only APIs such as List.getFirst(). A change like that passes here and only fails when a release tag is pushed, which is the case #125 is meant to catch.
Suggest using the same JDK in both workflows, and setting <maven.compiler.release>20</maven.compiler.release> (or <release>20</release> in the compiler plugin) so javac rejects newer APIs regardless of the JDK.
There was a problem hiding this comment.
Aligned both workflows on JDK 21 and replaced source/target settings with Maven release 20 in 3043e40. Maven packaging passed, and a focused compile check confirms Java 21-only List.getFirst() is rejected. Extension installation also passed.
…o codex/issue-125-test-workflow
Co-authored-by: Codex <noreply@openai.com>
CatarinaGamboa
left a comment
CatarinaGamboa
left a comment
There was a problem hiding this comment.
With both push: branches: [**] and pull_request, same-repo PRs run Checks twice (e.g. each codex/* branch shows a push + pull_request pair). The concurrency group uses github.ref, which differs between the two (refs/heads/… vs refs/pull/N/merge), so it does not dedupe. Maybe limit push to main, or skip same-repo pull_request like the integration job does?
Co-authored-by: Codex <noreply@openai.com>
rcosta358
changed the title
|
Addressed the duplicate-run review in cfae3f8: Test now runs on every pull request and on pushes to main, while reusable release checks remain enabled. The required Checks job executes without a skip condition, and separate ref-based concurrency protects PR runs from cancellation by main or tag runs. Independent review, trigger assertions, extension installation, and the new CI run passed. |
Part of #126. Depends on #138. Publish the exact VSIX produced by the reusable checks workflow. Prepare releases through a version-bump PR, then tag the merged main commit with `./release.sh --tag VERSION`. Retain checked artifacts for one day. A repository administrator still needs to require Checks on main. Validated shell/workflow syntax, extension installation, and a disposable repository simulation of the release PR and post-merge tag flow. 🤖 Generated with [Codex](https://openai.com/codex/) --------- Co-authored-by: Codex <noreply@openai.com>
## Description Closes #134. Add weekly and manual Windows/macOS runs for client and server unit tests plus VS Code stable integration tests, with logs uploaded on failure. The diff contains only `platform-tests.yml`. ## Related Issues Depends on #146. The existing prerequisite PRs now form one chain through #145, #144, #143, #141, #142, #140, #139, #138, and #137 to main. The schedule becomes active when merged to the default branch. Validation: 32 client tests, 24 server tests, lint, production/test TypeScript checks, and extension installation passed. Stable and minimum VS Code integration passed in [PR CI](https://github.com/liquid-java/vscode-liquidjava/actions/runs/37541530280). Fresh [Windows/macOS validation](https://github.com/liquid-java/vscode-liquidjava/actions/runs/37541530341) passed on the final commit, including both unit-test suites and VS Code integration. 🤖 Generated with [Codex](https://openai.com/codex/) --------- Co-authored-by: Codex <noreply@openai.com>
2 participants
Closes #125. Depends on #137.
Run lint, TypeScript, the server build, and packaged runtime checks on every pull request and on pushes to main. Keep reusable checks for publishing, and execute the required Checks job without a skip condition.
Validated workflow trigger coverage, Java 20 API targeting under JDK 21, packaging, and extension installation.
🤖 Generated with Codex