QVAC-18149: Improve addon-cpp testing, fix JS callback lifetime, expose/workaround bare double creation issue, harden addon-cpp workflows#1825
Merged
Conversation
jesusmb1995
force-pushed
the
temp-js_addon_cpp_callback_issue
branch
from
3a9819d to
3be07bc
Compare
jesusmb1995
changed the title
jesusmb1995
force-pushed
the
temp-js_addon_cpp_callback_issue
branch
from
bd17475 to
2036b2c
Compare
jpgaribotti
reviewed
Apr 30, 2026
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
Author
|
Last CI run https://github.com/tetherto/qvac/actions/runs/25170715644/job/73789136180 (manual new tests). PR at this point https://github.com/tetherto/qvac/actions/runs/25170408320/job/73787997234?pr=1825 |
Contributor
Author
|
CI tests at this point: include new js tests https://github.com/tetherto/qvac/actions/runs/25171423454/job/73791643736?pr=1825 |
Proletter
previously approved these changes
Apr 30, 2026
This comment has been minimized.
This comment has been minimized.
Run addon JS integration packages across desktop targets so callback lifetime and platform-specific runtime issues are exercised in PR checks.
Keep callback state alive until the libuv async handle is closed so pending JS output delivery cannot observe freed addon storage during teardown.
Add a minimal integration package that exercises first double creation through js::Number and keeps js_create_int32 as a control across the JS test matrix.
Route addon double creation through js::Number so win32 can burn the first js_create_double call observed to produce an invalid value on GitHub Azure runners.
Use a function-local static initializer for the Windows-only js_create_double burn-in. This keeps the workaround process-wide without carrying template-level atomic state before returning the requested double value.
Limit the addon-cpp JS PR tests to read-only repository access and avoid release secrets, inherited secrets, and persisted checkout credentials. Install fixture dependencies without lifecycle scripts so PR-controlled package code cannot run during setup.
Run addon-cpp JS tests from the unprivileged pull_request workflow so PR-controlled code is not executed from pull_request_target. Keep a lightweight verify-label gate for external forks while leaving the privileged workflow focused on authorization and native tests.
Contributor
Author
|
CI checks/run at this point https://github.com/tetherto/qvac/actions/runs/25173832566/job/73800394212?pr=1825 |
This comment has been minimized.
This comment has been minimized.
Contributor
Author
Move PR-controlled native test execution out of pull_request_target so cache restore and builds run without write-capable credentials, inherited secrets, or PAT-backed checkouts. Keep pull_request_target limited to clearing the verify label when new commits arrive, and make unverified PR events skip the native/JS matrices without failing the workflow.
Contributor
Author
This comment was marked as resolved.
This comment was marked as resolved.
gianni-cor
approved these changes
Apr 30, 2026
tamer-hassan-tether
approved these changes
May 1, 2026
Contributor
|
/review |
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
9 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.
addon-cpp. Add workflows that executebaretests. Expose bugs.verifylabel to run new commit checks (to be as safe as possible).JS Double Issue
on this test:
Manual CI run: https://github.com/tetherto/qvac/actions/runs/25158571461/job/73746732241 exposing failure.