[CI] run the instrumented tests of the Android API on Ubuntu

[myungjoo]

• What this fixes: the Android instrumented tests (java/android/nnstreamer/src/androidTest) never run in CI and are not even compiled, so a regression in the JNI wrapper, or in the C-API behavior it relies on, reaches main unnoticed. This came up in the review of [Android] release native handles before their callback data #696 (H5/H6 of [C-Api] Memory-safety audit of C sources: 47 verified issues (6 HIGH / 15 MEDIUM / 26 LOW) — tracking checklist #690), whose regression tests are instrumented tests.
• How: the JNI wrapper already builds on Ubuntu from the same sources (meson -Djava-home), so a new job builds it, compiles the tests against small stubs of the Android classes they use, and runs them on a desktop JVM, skipping the tests that need a device (java/host-test/exclude.txt). Separately, the x86_64 Android job now also compiles the instrumented tests (assembleDebugAndroidTest).
• Result: in CI, 164 tests pass and 10 are excluded in a 2.5 minute job; the Android test build adds about 43 s. Against the code that [Android] release native handles before their callback data #696 fixes, the same tests crash the JVM (SIGSEGV in nns_sink_data_cb) and fail the job, so the bug class is now caught before merge.

What it looks like

The new check is Android JNI test on Ubuntu. It installs nnstreamer from ppa:nnstreamer/ppa, builds ml-api with -Djava-home, and runs java/test-nnstreamer-ubuntu.sh. The script prints the JUnit result, and the native stack from the crash report if the JVM crashes. It runs on pull requests that change c/**, java/**, or the workflow file, and can also be run locally:

meson setup build -Djava-home=$JAVA_HOME -Denable-test=false -Denable-ml-service=false
ninja -C build
bash java/test-nnstreamer-ubuntu.sh --ml_api_dir=. --build_dir=build

The 10 excluded tests need an Android-only element (amcsrc), an Android native window for the video sink, the Android application context check, or the test assets shipped in the Android test package (png, video, tflite model). The four video-sink cases are excluded together: with glimagesink present but no display the pipeline does not start, so the three negative ones would pass on that failure rather than on the setSurface() call they are written for. APITestMLService is not built, as the host build has no ml-service.

Limitations: the host job does not cover the __ANDROID__ code paths, ART's stricter JNI checks, or GStreamer 1.24 (Ubuntu 22.04 ships 1.20); an emulator job would, and was deliberately not chosen for per-PR CI (runtime, flakiness). The job depends on the nnstreamer PPA daily build staying installable.

Details for reviewers

Files. .github/workflows/android-jni-test-ubuntu.yml (new job), java/test-nnstreamer-ubuntu.sh (build and run), java/host-test/stub/** (Android stubs: Context, AssetManager, Build, InstrumentationRegistry, AndroidJUnit4, Surface*), java/host-test/src/.../HostTestRunner.java (JUnit runner honoring the exclude list), java/host-test/exclude.txt, and for the Android side java/build-nnstreamer-android.sh (--build_test) with .github/actions/android-build/action.yml.

Exclude list. An entry that matches no test fails the run, so the list cannot silently go stale when a test is renamed or removed.

Verification. Besides the green CI run, each failure mode was checked locally (WSL, Ubuntu 22.04, GStreamer 1.20.3, OpenJDK 11):

Scenario	Result
This branch	164 passed, 10 excluded, exit 0
Stale exclude entry	exit 1, "Exclude entry does not match any test"
JNI mutation: sink callback skips the Java call	exit 1, 12 failures
JNI mutation: custom-filter destroy skips the unregister	exit 1, 8 failures
Syntax error in an androidTest file	exit 1, "Failed to compile the tests."
#696 tests without the #696 fix	exit 134, SIGSEGV in nns_sink_data_cb ← cb_sink_event, native frames printed
#696 tests with the #696 fix	174 passed, exit 0

shellcheck reports nothing on the new script, and the Android build script's warning count is unchanged.

Two runner-specific fixes found by CI: libunwind-dev must be requested explicitly, as the nnstreamer workflows do (a082c31); testSetNullSurface fails where glimagesink exists but there is no display, so it is excluded (a5d0edd).

Not enabled: -Xcheck:jni. It reports about 33k "JNI call made without checking exceptions" warnings from existing code (CallObjectMethod/CallIntMethod without an ExceptionCheck, e.g. in nns_parse_tensors_data). That is existing JNI hygiene debt and deserves its own issue.

Side effect: editing .github/actions/android-build/action.yml changes the GStreamer-Android cache key, so the next runs download that package again until main saves a new cache.

🤖 Generated with Claude Code

[myungjoo]

This is a code review produced by another agent (a Claude code-review sub-agent), transcribed onto this PR as a comment. It is not from a human maintainer.

Summary

Reviewed the diff (5 files changed, 1 new workflow, host-test harness + stubs, test-nnstreamer-ubuntu.sh, and the --build_test flag added to build-nnstreamer-android.sh / action.yml), cross-checked it against java/android/nnstreamer/src/{main,androidTest}, java/meson.build, the existing build-nnstreamer-ubuntu.sh sed pattern it reuses, the other .github/workflows/*.yml files, and the repo's branch-protection/ruleset configuration via the GitHub API.

Does it solve the stated problem? Yes. The instrumented tests under androidTest were previously never compiled or run in CI. This PR makes them compile and run on a desktop JVM against the real JNI wrapper (same C sources as Android, built via meson -Djava-home), with a documented, self-enforcing exclude list for the tests that genuinely need a device/display/assets. I traced the exclusions against the actual test bodies and they're all justified (amcsrc/testAvailableElement's assertTrue(isElementAvailable("amcsrc")), the @BUILD_ANDROID@-gated context check, PNG/TFLite asset tests, testSetNullSurface). APITestMLService is correctly the only test class skipped for the missing ml-service build, and no other test file references MLService. The android.os.Build/Context/Surface stubs line up with every android.* import actually used in main/androidTest — nothing is missing.

Regression risk to other modules: Low. --build_test=yes is only threaded through the x86_64 leg of the existing PR-triggered android.yml matrix build (confirmed: no other workflow invokes the composite action with an ABI matrix), and it only runs gradlew assembleDebugAndroidTest (packaging, not execution) after a successful nnstreamer:build, gated behind ${android_lib_build_res} -eq 0, so it can't make a previously-passing build fail for unrelated reasons. The new workflow is a separate, additive job scoped to c/**/java/** paths. All 11 checks are green on a5d0edd.

Size vs. topic: Reasonable. Every changed/added file is either the new CI job, the host-test harness (runner + minimal Android stubs, ~250 lines total) needed to make that job possible, or the two small hooks (--build_test flag, one added CLI arg) needed to compile the tests on the Android side. Nothing outside java/** and .github/** is touched except the one intentional cache-key side effect the PR description itself calls out.

Findings

[Medium] CI green does not currently mean "blocks merge." I checked main's branch protection via the GitHub API (repos/nnstreamer/api/branches/main/protection) and the repo's rulesets: only DCO is a required status check; there is no ruleset requiring status checks either. That means the new Android JNI test on Ubuntu job — like every other existing CI job (GBS builds, test, build (x86_64), etc.) — is advisory only and does not block the "Merge" button if it fails. This directly undercuts the PR's own stated goal ("...so the bug class is now caught before merge"): today it's caught and visible, but not enforced. This isn't something this PR's files can fix by themselves (branch protection is a repo setting), but it's worth a maintainer follow-up to add this job (and ideally the other build/test jobs) to the required status checks, or the safety net this PR builds has a hole a maintainer can merge through.

[Medium, please double-check] Possible vacuous pass in three sibling surface tests. a5d0edd excludes APITestPipeline#testSetNullSurface because "glimagesink exists but there is no display" makes it fail on this headless runner. Looking at APITestPipeline.java, three other tests share the exact same shape (isElementAvailable("glimagesink") → build a glimagesink pipeline → pipe.start() → do the thing under test → catch (Exception e) { /* expected */ }): testSetSurfaceEmptyName_n, testSetInvalidSurface_n, testSetSurfaceNullName_n. Since these expect some exception (rather than fail() on any exception, as testSetNullSurface does), if pipe.start() itself throws on this headless runner before the test reaches the setSurface(...) call it's meant to exercise, the test would still report "pass" — just not for the reason it's supposed to. Given testSetNullSurface demonstrably fails at start()/no-display, it seems plausible these three also never get past start(), and are currently green for the wrong reason (i.e., they aren't really validating the invalid-surface/invalid-name argument checks on this job). Worth confirming locally (e.g., temporarily changing their catch to print whether the exception happened at start() vs. the intended call) and either excluding them with the same rationale as testSetNullSurface, or confirming they do fail at the intended line and this concern doesn't apply.

[Low] Workflow path filters miss top-level build files. android-jni-test-ubuntu.yml triggers on c/**, java/**, and its own file, but not on root meson.build/meson_options.txt. A change to, say, the java-home or enable-ml-service option definitions there wouldn't retrigger this job even though it could affect the build this job exercises. Low impact since such changes are rare and the unconditional android.yml build job would still catch outright build breakage.

[Low / Nit] Self-disclosed cache churn, confirmed real. Editing .github/actions/android-build/action.yml changes hashFiles('.github/actions/android-build/action.yml'), which is the gst_android_pkg cache key (action.yml:37,108). Confirmed this will cause a cache miss and a fresh ~re-download of the GStreamer-Android package on main until the cache is resaved there. The PR description already flags this explicitly, so this is just independent confirmation, not a new problem.

[Nit] Local-test discoverability. java/README.md has no mention of java/test-nnstreamer-ubuntu.sh or the host-test harness. The PR description documents the exact commands to run it locally, which is good, but that context lives only in the PR, not the repo. A one- or two-line pointer in java/README.md would help future contributors find it without digging through PR history.

Other checks

• Documentation/architecture: No public API or architecture changed (same JNI wrapper sources, no new C/Java API surface); no doc updates are required for this PR, and none are missing.
• Future-regression detection: The exclude-list design is a good safeguard by itself — HostTestRunner fails the run if any exclude.txt entry doesn't match a real test (java/host-test/src/org/nnsuite/nnstreamer/HostTestRunner.java), so a renamed/removed excluded test can't silently go stale, and the job is scoped to re-run whenever c/**/java/** changes, which is exactly the code whose future changes should be caught (module scope aside from the Medium/Low path-filter and branch-protection points above).
• Stub correctness: Cross-checked every android.* import in main/java and androidTest/java against java/host-test/stub/**; all are covered (Context, AssetManager, Build, InstrumentationRegistry, AndroidJUnit4, Surface, SurfaceHolder, SurfaceView), and the sed substitutions in test-nnstreamer-ubuntu.sh correctly mirror the pre-existing pattern in build-nnstreamer-ubuntu.sh (only applied to the main/*.java copies before the real-android-type androidTest/*.java files are copied in), so this isn't new/untested technique.

Verdict

Verdict: ok to merge

No Critical or High-severity issues found in the changed code itself; CI is green and the added coverage is demonstrably real (the PR's own mutation-testing table shows genuine failures being caught, and I independently traced every exclusion to a legitimate host limitation). The Medium items above are both non-blocking follow-ups: one is a repo-setting gap (branch protection) outside this PR's file set, the other is a "please double check locally" question about three specific tests rather than a confirmed bug.

[myungjoo]

Addressing the review in #697 (comment). Three of the five items are fixed in ce3ece2; the remaining two need no change here.

1. [Medium] CI does not actually block a merge — correct, and outside this PR. main requires only the DCO check (plus one approving review); no other status check is required, so any CI failure, including this new job, can still be merged. Reported to the maintainer; it is a repository setting, not something this PR can fix. What this PR does change is that a JNI regression now fails a check instead of going unnoticed.
2. [Medium] The other three surface cases could pass vacuously — confirmed and fixed. testSetSurfaceNullName_n, testSetSurfaceEmptyName_n and testSetInvalidSurface_n wrap the whole scenario, including pipe.start(), in one try block and treat any exception as the expected one. With glimagesink present but no display, start() already throws, which is exactly why testSetNullSurface failed on the runner, so those three would have passed on the wrong exception. They are excluded with the other video-sink case and the reason is recorded in exclude.txt. The host run is now 164 passed, 10 excluded.
3. [Low] Path filter — fixed: the workflow now also triggers on meson.build and meson_options.txt, which build the JNI wrapper.
4. [Low/Nit] Cache key change — no change; it is a one-time re-download, already noted in the PR description.
5. [Nit] Documentation — fixed: java/README.md was empty and now describes the three scripts, including how to run the host test locally.

[myungjoo]

This is a code review produced by another agent (a Claude code-review sub-agent), transcribed onto this PR as a comment. It is not from a human maintainer.

Scope

Re-review of ce3ece2 (5 commits total), the follow-up to the earlier review at #697 (comment) and the author's reply at #697 (comment). I re-checked whether the three "fixed" items actually resolved the finding, whether the two "no change" items still hold, and reviewed ce3ece2 plus the overall diff on its own merits (workflow YAML, test-nnstreamer-ubuntu.sh, HostTestRunner.java, the android/* stubs, build-nnstreamer-android.sh, java/README.md, exclude.txt), cross-referenced against APITestPipeline.java, Pipeline.java, meson_options.txt, the other .github/workflows/*.yml files, and live CI/check-run data via the GitHub API.

Does it solve the stated problem? Yes, unchanged from the previous round: the instrumented tests now compile and run on a desktop JVM against the real JNI wrapper, with a self-enforcing exclude list.

Verification of the three "fixed" items

1. Vacuous-pass fix (testSetSurfaceNullName_n, testSetSurfaceEmptyName_n, testSetInvalidSurface_n now excluded). Confirmed correct, and I traced it one level deeper than the original finding: I read Pipeline.setSurface() (java/android/nnstreamer/src/main/java/org/nnsuite/nnstreamer/Pipeline.java:416-436). The null/empty name check (if (name == null || name.isEmpty())) is plain Java and would run on the host build, but all three tests call pipe.start() on a glimagesink pipeline before calling setSurface(...), and start() already throws with no display — so the name-check line is never reached on this runner regardless. testSetInvalidSurface_n is doubly justified: the surface-type/isValid() check it exercises (Pipeline.java:424-430) is wrapped in @BUILD_ANDROID@ guards that test-nnstreamer-ubuntu.sh comments out entirely for the host build (sed -i "s|@BUILD_ANDROID@|//|"), so that code path doesn't even compile into the host test — there's no way this test could validate anything host-side. Excluding all four video-sink tests is the right call; nothing left to fix here.
2. Path filter (meson.build, meson_options.txt added). Confirmed present in .github/workflows/android-jni-test-ubuntu.yml and both options (java-home, enable-ml-service, enable-test) do exist in meson_options.txt, so the filter now covers the build inputs the job depends on.
3. java/README.md documentation. Confirmed added and accurate: the described --build_test=yes/--run_test=yes flags match build-nnstreamer-android.sh, and the quoted meson setup ... && ninja ... && bash java/test-nnstreamer-ubuntu.sh ... sequence matches both the option names in meson_options.txt and what the new workflow itself runs.

The two items left unchanged (branch protection not requiring this check; the one-time GStreamer-Android cache-key churn) are correctly reasoned: branch protection is a repo setting outside this PR's file set, and the cache churn was already self-disclosed and is a one-time cost, not a correctness issue.

New findings (this round)

[Low] The new job's check shows up simply as "test", not by a descriptive name. .github/workflows/android-jni-test-ubuntu.yml defines jobs: test: with no job-level name:. I checked: every other job in every workflow in this repo sets an explicit name: distinct from the job id specifically so the check list is legible — e.g. gbs_build.yml job id build → name: Tizen GBS build on Ubuntu, spell-checker.yml job id typos-check → name: Spell Check with Typos, static.check.yml job id simple_script_checkers → name: Static checks. I confirmed via the Checks API on ce3ece2 that this job is in fact the only check in the whole run list literally named test (gh api repos/nnstreamer/api/commits/ce3ece2.../check-runs), so there's no live collision today, but the name is generic and easy to collide with in the future, and it is exactly the kind of ambiguous name that makes it harder to pick this job by name later if the maintainer follows up on the earlier Medium finding and adds it as a required status check. A one-line name: Android JNI host test under jobs.test would fix it and matches repo convention.

[Informational, not a defect] One check is still pending at review time. Tizen GBS build on Ubuntu (x86_64, --define "unit_test 1") was still in_progress as of this review; all other checks on ce3ece2, including the new Android JNI test on Ubuntu job, are green. This job is unrelated to this PR's changed files (it's the pre-existing GBS/Tizen unit-test build), so it's not a finding against this diff, just worth confirming green before merge.

Other checks

• Regression risk / module boundaries: unchanged from the previous round — --build_test=yes is only wired into the x86_64 leg of the existing Android matrix build, only runs assembleDebugAndroidTest (packaging, gated on ${android_lib_build_res} -eq 0, and its own failure correctly propagates through android_lib_build_res to the script's final exit), and the new workflow is additive. No change to that assessment after re-reading build-nnstreamer-android.sh in full.
• Size vs. topic: still reasonable; ce3ece2 itself only touches the workflow path filter, the exclude list, and the README — proportionate to "close out prior review findings."
• Architecture/API docs: no API or architecture change in this PR; no doc gap beyond the java/README.md addition already made.

Verdict

Verdict: ok to merge

No Critical/High issues. The previous round's two Medium findings are resolved (vacuous pass) or correctly deferred as out-of-scope (branch protection is a repo setting, not this PR's to fix). The one new item (Low) is cosmetic and non-blocking — it doesn't affect current CI behavior, only the discoverability of the check's name.

[myungjoo]

Addressing the review in #697 (comment).

• [Low] The job shows up as test in the check list. Fixed in 5027a7e: the job is now named Android JNI host test, following the convention of the other workflows. The point about required status checks is the reason it matters, so it is worth the one line.
• [Medium, kept] Branch protection does not require the CI checks. Still outside this PR; reported to the maintainer. main requires only DCO plus one approving review, so making this job (or the existing ones) actually block a merge is a repository setting.
• [Low/Nit, kept] GStreamer-Android cache key. One-time re-download, already noted in the description.

The three items fixed in ce3ece2 were verified by the reviewer against the code rather than the changelog, which matches what I intended: in particular, setSurface()'s invalid-surface check is behind @BUILD_ANDROID@ and therefore not even compiled on the host, so excluding those cases was the right call rather than a workaround.