You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@opennlp.apache.org by "rzo1 (via GitHub)" <gi...@apache.org> on 2026/06/25 13:20:31 UTC

[PR] Merge shell tests into the Java CI pipeline (opennlp)

rzo1 opened a new pull request, #1120:
URL: https://github.com/apache/opennlp/pull/1120

   ## What
   
   Runs the shell (bats/Pester) tests as a stage **after** the regular build in `maven.yml`, and removes the standalone `shell-tests.yml` workflow.
   
   ## Why
   
   Previously the distribution was built **9×** per run: 6 in `maven.yml` (3 OS × 2 JDK) plus 3 more in `shell-tests.yml` (one per OS), purely to produce the tarball the shell tests run against.
   
   The binary distribution is pure-Java and platform-independent, so it is now:
   
   1. Built **once** in the `build` job (Ubuntu / JDK 21) and uploaded via `actions/upload-artifact`.
   2. Downloaded by the per-OS shell-test jobs (`needs: build`) via `actions/download-artifact`, which then extract and run bats (Ubuntu/macOS) / Pester (Windows).
   
   This drops the 3 redundant Maven builds per run.
   
   ## ASF policy note
   
   `actions/upload-artifact` and `actions/download-artifact` are in the `actions/*` namespace, which is **automatically allowed** under the [ASF GitHub Actions policy](https://infra.apache.org/github-actions-policy.html) (alongside `apache/*` and `github/*`). No INFRA allowlist change is required.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@opennlp.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Merge shell tests into the Java CI pipeline (opennlp)

Posted by "krickert (via GitHub)" <gi...@apache.org>.
krickert commented on code in PR #1120:
URL: https://github.com/apache/opennlp/pull/1120#discussion_r3481044137


##########
.github/workflows/maven.yml:
##########


Review Comment:
   Found a potential issue: the shell tests may be skipped when the build matrix has partial failures.  This doesn't look intentional.. 
   
   The three new shell-test jobs (`test-unix-shell-ubuntu`, `test-unix-shell-macos`, `test-windows-shell`) all declare:
   
   ```yaml
   needs: build
   ```
   
   My understanding:
   
   The `build` job is a matrix of 6 combinations (`ubuntu`/`macos`/`windows` × `java 21`/`25`) with `fail-fast: false`. In GitHub Actions, `needs: <matrix-job>` waits for the entire matrix to succeed. If any single leg fails (even an unrelated one like `windows-latest` + Java 25), the shell-test jobs are marked as skipped—even though the `ubuntu-latest`/`java 21` leg that produces the `opennlp-distribution` artifact succeeded and uploaded it.
   
   The comments in the workflow (lines 60-62 and 74-75) indicate the intent is to reuse the distribution from the Linux/JDK 21 build. This skipping behavior is probably unintentional?
   
   Assuming this is the intent, I can think of two ways to fix this:
   - Introduce a separate, non-matrix job (e.g., `build-distribution`) that only runs on `ubuntu-latest` + Java 21, uploads the artifact, and have the shell tests depend on that job instead.
   - Or keep the current matrix but make the shell-test dependency more granular (e.g., via an `if:` condition that checks whether the required artifact exists).
   
   Thoughts?
   
   



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@opennlp.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Merge shell tests into the Java CI pipeline (opennlp)

Posted by "rzo1 (via GitHub)" <gi...@apache.org>.
rzo1 commented on PR #1120:
URL: https://github.com/apache/opennlp/pull/1120#issuecomment-4809305290

   Good catch on the mechanics — you're right that `needs: build` waits on the whole 6-leg matrix, so if any leg fails the shell-test jobs get skipped. But that's actually the behavior I want here, and it ties into the whole point of this PR.
   
   The goal is to cut CI load so we don't pile up queues when several PRs hit at once. Today the distribution gets built 9× per run (6 in `maven.yml` + 3 in `shell-tests.yml`); this PR drops that to building it once on Linux/JDK 21 and reusing the artifact. Gating the shell tests on a fully-green matrix fits the same intent: if any build leg is red, the run is already failing, so spinning up three more OS runners to shell-test a build we know is broken just burns minutes we're trying to save. The skip can't ever mask a failure — it never turns a red run green — so we lose nothing on the safety side; we only skip shell coverage on a run that's already failing for another reason.
   
   Your `build-distribution` job idea would decouple it, but I'd rather not: it adds a runner back and would let shell tests go green while the main matrix is red, which is a more confusing signal than a clean skip.
   
   Thanks for the careful review!


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@opennlp.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org