fix(tts-ggml): signal explicit skip for benchmark shims on mobile - #2886
Closed
tobi-legan wants to merge 14 commits into
Closed
tobi-legan wants to merge 14 commits into
tobi-legan wants to merge 14 commits into
Conversation
The mobile test harness (qvac-test-addon-mobile PR #49) now requires an explicit skip signal to distinguish intentional 0/0 (benchmark not enabled) from accidental 0/0 (addon crash / dlopen failure). Set global.__QVAC_TEST_SKIPPED = true in both RTF and streaming benchmark shims when QVAC_TTS_GGML_RUN_BENCHMARK_ON_MOBILE is not set. The harness checks this flag and treats the result as PASS (skipped) instead of FAIL. Without this flag, 0/0 results correctly fail — catching the async dlopen crash scenario that previously slipped through as a false green. Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
Review StatusCurrent Status: ❌ PENDING Pending reviews: Needs 1 Management or Team Lead, and 1 more from Management, Team Lead, or Member. |
Co-authored-by: Cursor <cursoragent@cursor.com>
The global.__QVAC_TEST_SKIPPED flag doesn't reliably propagate across
bare-pack module scopes. Add module.exports = { __QVAC_SKIPPED: true }
so the mobile harness can read the skip signal directly from require()'s
return value — no cross-scope global needed.
Co-authored-by: Cursor <cursoragent@cursor.com>
Three new mechanisms: 1. ci-router composite action: reads PR labels, outputs routing flags. - "prebuilds" → prebuilds + lint + sanity - "run-desktop-addon-tests" → prebuilds + cpp-tests + desktop integration - "run-mobile-addon-tests" → prebuilds + mobile (requires "verified") - workflow_dispatch → everything runs 2. detect-native-changes composite action: diffs PR base/head for native files (*.cpp, *.h, CMakeLists.txt, vcpkg.*). Outputs native_changed flag + deterministic hash for cache key. 3. Prebuild caching via actions/cache: when native files are unchanged, restores prebuilds from cache instead of running the 9-platform matrix. Cache key: prebuilds-llm-pr-<number>-<native_hash>. First run always builds (no cache yet). Native file change forces fresh rebuild (cache key changes). Applied to on-pr-llm-llamacpp.yml only. Other addons can adopt by adding ci-router + detect-native-changes jobs and updating if: conditions. Co-authored-by: Cursor <cursoragent@cursor.com>
…ed main merge) Co-authored-by: Cursor <cursoragent@cursor.com>
The "verified" label now enables all CI stages (prebuilds + desktop + mobile) so existing PRs continue to work without disruption. The new labels (prebuilds, run-desktop-addon-tests, run-mobile-addon-tests) work in parallel. Once the team validates the new labels post-merge, a follow-up PR will remove verified from the ci-router and transition fully to the new label scheme. Co-authored-by: Cursor <cursoragent@cursor.com>
1. merge-guard: build-status now accepts prebuild success OR cache hit 2. detect-native-changes: fetch HEAD_SHA explicitly before diffing (fixes fork PRs where HEAD isn't in local refs). Fail loudly on errors instead of silently defaulting to false. 3. native_hash: computed from PR HEAD tree via git ls-tree (not base branch find+cat). Correct and secure — reads tree object without checking out PR code. 4. prebuild-cache-save: runs on ANY successful prebuild (removed native_changed gate). First build on a JS-only PR now populates the cache for subsequent pushes. 5. Removed unused composite action files (ci-router, detect-native- changes) — workflow inlines everything. Single source of truth. 6. Added permissions (contents: read) and timeout-minutes to all new jobs per devops rules. 7. Added cache-poisoning mitigation comment on prebuild-cache-save (per-PR-number key scoping). Co-authored-by: Cursor <cursoragent@cursor.com>
1. Fork PRs: when git fetch origin SHA fails (fork-only commit), fall back to fetching from the fork repo by ref via github.event.pull_request.head.repo.full_name. HEAD_SHA is updated to FETCH_HEAD so diff and ls-tree use the correct commit. 2. CMakeLists.txt: added $WORKDIR/CMakeLists.txt to the git ls-tree paths so package-root CMake changes invalidate the cache key. Previously only addon/ subdirectory CMakeLists were hashed. Co-authored-by: Cursor <cursoragent@cursor.com>
bare-pack module scopes may not have `global` defined as a variable, causing `typeof global !== 'undefined'` to be false and skip signal never propagating. Use globalThis (always available) and also set exports.__QVAC_SKIPPED for bundlers that don't respect module.exports reassignment. Co-authored-by: Cursor <cursoragent@cursor.com>
Label scheme now matches each stage independently: - verified → sanity-checks + cpp-lint (+ backward compat: everything) - prebuilds → only prebuild/cache - run-cpp-addon-tests → C++ tests (prebuilds as dependency) - run-desktop-addon-tests → desktop integration (prebuilds as dependency) - run-mobile-addon-tests → mobile (requires verified + prebuilds) New ci-router outputs: run_verified_checks, run_cpp_tests (in addition to existing run_prebuilds, run_desktop, run_mobile). sanity-checks and cpp-lint now gated on run_verified_checks (not run_prebuilds). prebuild no longer depends on sanity-checks since they are independent stages. Co-authored-by: Cursor <cursoragent@cursor.com>
Debug logging confirmed bare-pack isolates globalThis/global per module, making all previous approaches invisible across require(). The console object IS shared (verified by shim logs flowing through the backend.cjs override). Use console as the cross-module signal. Co-authored-by: Cursor <cursoragent@cursor.com>
Mirror the addon-mobile change that marks 0-sub-test tests pending via this.skip(). The WDIO afterTest hook now checks test.pending and records status 'skipped' (instead of inferring passed/failed) so the on-device test-results.json summary reflects intentional skips. Co-authored-by: Cursor <cursoragent@cursor.com>
The mobile benchmark shims previously signalled an intentional skip by setting console/global/module.exports flags, which the harness could not read reliably. They now call the harness-provided global.skipMobileTest, which registers a real brittle skipped test and tags the shared runner. This restores the harness 0/0 = FAIL safety net: an intentional skip is now a real (skipped) registered test, while a module that registers nothing at all (e.g. a silent addon-load crash) stays a failure instead of a green skip. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SBoRDUf9ZQeE5LFH5rwnWs
A backend global (skipMobileTest) is not visible inside these require()'d shim modules (run 28272783534 hit the fallback and failed 0/0). brittle is the channel that crosses into the bundled runtime, so the shims now register a real brittle skip directly. The harness wraps brittle.skip to report it as skipped; a module that registers nothing stays a 0/0 FAIL. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SBoRDUf9ZQeE5LFH5rwnWs
Contributor
Author
|
Closing — superseded by #2918. #2918 adopts file-driven mobile benchmark selection (test/mobile/test-groups.json + perf-tests.json), so the env-gated RTF/streaming benchmarks are simply excluded from normal mobile runs and never produce a 0/0. That approach needs no qvac-test-addon-mobile harness change, so this PR's explicit __QVAC_TEST_SKIPPED signal (and its coordinated harness PR qvac-test-addon-mobile#49) are no longer needed. Tracked by QVAC-21371, now resolved on main via #2918. |
This branch had an error being deployed
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
1 participant
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.
Summary
global.__QVAC_TEST_SKIPPED = trueto the RTF and streaming benchmark mobile shims whenQVAC_TTS_GGML_RUN_BENCHMARK_ON_MOBILEis not setContext
PR #47 in qvac-test-addon-mobile made
0/0 = FAILto catch the 2026-06-09 TTS dlopen outage that previously slipped through as a false green. However, the benchmark shims intentionally produce 0/0 when the env var isn't set. PR #49 adds support for an explicit__QVAC_TEST_SKIPPEDglobal flag that shims can set to signal "this is intentional, not a crash."Changes
packages/tts-ggml/test/integration/rtf-benchmark.test.js__QVAC_TEST_SKIPPEDwhen skippingpackages/tts-ggml/test/integration/streaming-benchmark.test.js__QVAC_TEST_SKIPPEDwhen skippingMerge order
If merged in reverse order, TTS-GGML benchmarks will temporarily fail (same as current main after #47).
Test plan
Made with Cursor