Skip to content

ci: run the package's own PHP and JS suites - #2

Merged
ddespinoza merged 2 commits into
mainfrom
ci/run-the-suites
Oct 3, 2026
Merged

ddespinoza merged 2 commits into
mainfrom
ci/run-the-suites

Conversation

@ddespinoza

Copy link
Copy Markdown
Contributor

What & why

aps-common had one workflow, docs.yml, so a green PR here only proved the markdown links resolve. This adds ci.yml, which runs the package's own suites on every PR and on main:

  • PHP tests: make test, which runs PHPUnit in Dockerfile.test, the image with intl.
  • JS tests: node --test js/*.test.js, the fold conformance corpus, on Node 22.

The README's consumer list is also corrected: estadistica and epidemiologia (JS half only) consume the package too. It now mentions the 🔗 Adopt: PRs the Integrator routine opens in each app after a merge here.

This is also the first PR the Claude reviewer sees in aps-common, since the webhook was added today, so it doubles as that live test.

Checklist

  • Title follows Conventional Commits.
  • The repository's own gate passes locally:
    • make test: 38 tests, 48 assertions, OK.
    • node --test js/*.test.js: 2 passed.
    • repo-docs.py check . --offline: 0 errors.
  • Docs updated where a fact changed (README: Consumers, Tests).
  • Small and reviewable; 1 approval before merge.

AI involvement

  • This PR was written with AI assistance → labeled ai-assisted.
  • Human-reviewed. Not yet: dani reviews and merges.

🤖 Generated with Claude Code

docs.yml was the only workflow, so a green PR proved only that the markdown links resolve. The README
now names all four consumers (epidemiologia takes the JS half only), the re-sync PRs the Integrator
opens after a merge, and the JS test command.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@ddespinoza ddespinoza added the ai-assisted PR/issue includes AI-assisted work; disclose in description label Oct 2, 2026

Copy link
Copy Markdown
Contributor Author

🤖 Claude review — ✅ Ready to merge

This adds .github/workflows/ci.yml with two jobs. The PHP job runs make test, which runs PHPUnit inside Dockerfile.test, so intl is present. The JS job runs node --test js/*.test.js on Node 22. This is the package gate the org rules name, and until now no workflow ran it here. The README's Consumers section is corrected too, and I checked it against the clones: territorio, farmacia and estadistica each commit all four surfaces under vendor/aps/common/src/ + src/aps/, and epidemiologia commits only src/aps/. It is one concern and it is small. src/ and js/ behaviour is unchanged, so no CHANGELOG line is needed. It has no secrets and touches no vendored file (repo-docs.py, profiles/). The PR carries the ai-assisted label and the AI disclosure.

Findings (none blocking)

  • Nit, README.md:14: the Status line still says "consumed by both apps". After the Consumers fix in this same PR, that sentence is stale. Worth rewording in the same edit.
  • Nit, README.md:71-72: "the apps' aps-drift gates go red". epidemiologia's CI has no aps-drift step, so its drift is only caught when someone runs make aps-drift by hand. "territorio's, farmacia's and estadistica's" would be exact.
  • Nit, .github/workflows/ci.yml:20-22: cancel-in-progress: true also applies to push on main. If two merges land close together, the first merge's run is cancelled and that commit never gets a result. One common fix is cancel-in-progress: ${{ github.event_name == 'pull_request' }}. This is optional.

Gate: CI green on 46782df: PHP tests ✅ (the log shows OK (38 tests, 48 assertions), including the accent/slug corpus cases, so intl is loaded), JS tests ✅, docs check ✅. Local gate: node --test js/*.test.js → 2 passed (Node 22.22). python3 .github/repo-docs.py check . --offline → 0 errors. make test was not run locally because this sandbox has no Docker daemon, so the CI run above is the PHP proof.
Suggested squash title: ci: run the package's PHP and JS suites on every PR


Generated by Claude Code

From review on #2: cancel-in-progress only for pull requests, so close merges each get a result; the
Status line no longer says 'both apps'; epidemiologia's CI has no drift step.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Copy link
Copy Markdown
Contributor Author

🤖 Claude review — ✅ Ready to merge

The new commit d976f41 addresses all three nits from round 1, and adds nothing else. ci.yml now cancels in-flight runs only for pull_request (cancel-in-progress: ${{ github.event_name == 'pull_request' }}), so each main commit keeps its own result. The comment above it says the same. In the README, the Status line now reads "consumed by all four apps (see Consumers)" instead of the stale "both apps". The drift sentence names territorio, farmacia and estadistica as the CI aps-drift gates, and says that epidemiologia's CI has no drift step. That matches the clones. Across the whole PR the claim matches the diff, and it is still one concern, small, with no src//js/ behaviour change (so no CHANGELOG line), no vendored files touched, and the ai-assisted label plus disclosure in place.

Findings

  • None.

Gate: CI on d976f41: JS tests ✅, docs check ✅. PHP tests was still in progress when this was posted. This commit changes no PHP, and make test passed on 46782df (OK (38 tests, 48 assertions)), but dani should confirm the PHP job is green before merging. Local gate: node --test js/*.test.js → 2 passed, 0 failed. python3 .github/repo-docs.py check . --offline → 0 errors. ci.yml parses as valid YAML. make test was not run locally because this sandbox has no Docker daemon.
Suggested squash title: ci: run the package's PHP and JS suites on every PR


Generated by Claude Code

@ddespinoza
ddespinoza merged commit e393212 into main Oct 3, 2026
3 checks passed
@ddespinoza
ddespinoza deleted the ci/run-the-suites branch October 8, 2026 16:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-assisted PR/issue includes AI-assisted work; disclose in description

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant