Run the test suites on pull requests - #73
Merged
Merged
Conversation
Nothing ran on a pull request except CodeQL's default scan. Unit tests, the JNI build and the submodule pointer were exercised only by the tag build, so a regression stayed reviewable but undetectable — #68 arrived carrying 13 new unit tests that would not have run anywhere before merge, and a speech-core pointer older than main, which reads as one unremarkable line in a 2,100-line diff and would have quietly reverted the worker-loop fix. The submodule check encodes that second case: the pointer may advance, but it may not retreat, and it has to exist upstream. A pointer reachable only from a fork or a local branch breaks every later checkout of this repository, and nothing was catching it.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Comment on lines
+18
to
+61
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| with: | ||
| submodules: recursive | ||
|
|
||
| - uses: actions/setup-java@v4 | ||
| with: | ||
| distribution: temurin | ||
| java-version: '17' | ||
|
|
||
| - uses: android-actions/setup-android@v3 | ||
|
|
||
| - name: Cache Gradle | ||
| uses: actions/cache@v4 | ||
| with: | ||
| path: | | ||
| ~/.gradle/caches | ||
| ~/.gradle/wrapper | ||
| key: gradle-${{ runner.os }}-${{ hashFiles('**/*.gradle.kts', 'gradle/libs.versions.toml', 'gradle/wrapper/gradle-wrapper.properties') }} | ||
| restore-keys: gradle-${{ runner.os }}- | ||
|
|
||
| # The SDK suite covers the download manifest; the app suite covers the | ||
| # demo's own logic, including text insertion. Neither ran on a PR before. | ||
| - name: Unit tests | ||
| run: ./gradlew :sdk:test :app:testDebugUnitTest | ||
|
|
||
| # Compiles the JNI bridge against the pinned speech-core, so a submodule | ||
| # that does not build with the current bridge fails here rather than at | ||
| # release time. | ||
| - name: Assemble | ||
| run: ./gradlew :sdk:assembleRelease :app:assembleDebug | ||
|
|
||
| - name: Upload test reports | ||
| if: always() | ||
| uses: actions/upload-artifact@v4 | ||
| with: | ||
| name: test-reports | ||
| path: | | ||
| sdk/build/reports/tests/ | ||
| app/build/reports/tests/ | ||
| retention-days: 7 | ||
|
|
||
| submodule-ancestry: |
Comment on lines
+66
to
+104
| if: github.event_name == 'pull_request' | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| with: | ||
| fetch-depth: 0 | ||
|
|
||
| - name: speech-core must not move backwards | ||
| run: | | ||
| set -euo pipefail | ||
| git fetch --quiet origin "${{ github.base_ref }}" | ||
| base=$(git rev-parse "origin/${{ github.base_ref }}:speech-core") | ||
| head=$(git rev-parse "HEAD:speech-core") | ||
| echo "base ${{ github.base_ref }}: $base" | ||
| echo "head: $head" | ||
|
|
||
| if [ "$base" = "$head" ]; then | ||
| echo "unchanged" | ||
| exit 0 | ||
| fi | ||
|
|
||
| # The pointer may legitimately advance. It may not retreat, and the | ||
| # commit has to exist upstream — a pointer only reachable from a fork | ||
| # or a local branch breaks every later checkout of this repository. | ||
| git clone --quiet --filter=blob:none --no-checkout \ | ||
| https://github.com/soniqo/speech-core.git /tmp/speech-core | ||
| cd /tmp/speech-core | ||
|
|
||
| if ! git cat-file -e "$head^{commit}" 2>/dev/null; then | ||
| echo "::error::speech-core $head does not exist upstream" | ||
| exit 1 | ||
| fi | ||
|
|
||
| if git merge-base --is-ancestor "$head" "$base"; then | ||
| echo "::error::speech-core moves backwards: $head is an ancestor of $base on ${{ github.base_ref }}" | ||
| exit 1 | ||
| fi | ||
|
|
||
| echo "speech-core advances or diverges forward" |
The Assemble step failed on its own pull request: CMake is handed ORT_DIR and LITERT_DIR, and nothing had created them. setup.sh fetches both AARs and the release workflow runs it before any Gradle task; CI has to do the same. Cached on the versions pinned in setup.sh, which the script already compares against the installed version.txt, so a hit makes the step a no-op instead of a repeated download. control-demo joins the test list to match what the release build covers.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Nothing ran on a pull request except CodeQL's default scan. Unit tests, the JNI build and the submodule pointer were exercised only by the tag build — so a regression was reviewable but not detectable, and the first time anything checked it was during a release.
Two concrete cases from #68, both real:
TextInsertionTestcases that would not have run anywhere before merge.speech-corepointer older than main, which would have silently reverted the worker-loop exception-safety fix from #126. In a 2,100-line diff that is one line reading-Subproject commit …/+Subproject commit …, and nothing distinguishes a legitimate bump from a reversion by looking at it.What runs
unit-tests—:sdk:testand:app:testDebugUnitTest, then:sdk:assembleReleaseand:app:assembleDebug. The assemble step matters independently: it compiles the JNI bridge against the pinned speech-core, so a submodule that no longer builds against the current bridge fails here rather than at release time. Test reports upload on failure.submodule-ancestry(PRs only) — comparesHEAD:speech-coreagainstbase:speech-coreand fails if the pointer is an ancestor of the base, i.e. moves backwards. It also verifies the commit exists insoniqo/speech-core, since a pointer reachable only from a fork or a local branch breaks every later checkout of this repository. Advancing the pointer stays allowed and unchanged pointers pass immediately.Notes
push: [main]as well aspull_request, so a direct commit to main is covered by the same gate.concurrencycancels superseded runs.connectedAndroidTest) are deliberately not here — they need an emulator and download ~1.2 GB of models, which is a separate decision about CI cost.