fix(test-harness): surface addon load failures instead of silence#20
Open
gmaclennan wants to merge 2 commits into
Open
fix(test-harness): surface addon load failures instead of silence#20gmaclennan wants to merge 2 commits into
gmaclennan wants to merge 2 commits into
Conversation
The runner only streamed NODEJS-MOBILE, the tag native-lib.cpp pumps node's stdout/stderr to. Two sources of failure log elsewhere and were dropped, so a broken run showed as unexplained silence: - node's own fatal-error reports go to tag `nodejs`, not through our stdout/stderr pipe, so an uncaught exception's message never appeared. - TestActivity's asset copy catches per-file errors and printStackTraces them to System.err, so a file missing on device was invisible. Diagnosing a failed rocksdb-native run needed unfiltered logcat on a local emulator to find `Error: ENOENT ... open 'test/fixtures/lock'` under `nodejs`. Add those tags plus AndroidRuntime. `-v raw` stays, so TAP output is unchanged and the sentinel parsing still works.
require-addon raises "Cannot find addon" when every candidate fails to *load*,
not only when none exist, so a prebuild that is present but cannot dlopen —
built without linking libnode.so so napi_* never resolves, wrong ABI — is
indistinguishable from one that was never installed. Both runners printed only
err.stack, which omits the cause.
Printing the cause is not enough either: the resolver assigns `cause` per
candidate, so it holds the last one's error, normally "Cannot find module" for
a root-level path that never existed. The real dlopen error is overwritten.
The error does carry the candidate list, so retry the candidates that exist on
disk and report why each failed, separating "no prebuild for this target" from
"prebuild present but broken". On a rocksdb-native run this turns a misleading
"Cannot find addon" into:
Addon diagnostics: candidate(s) present but failed to load:
.../fs-native-extensions/prebuilds/android-arm64/fs-native-extensions.node:
dlopen failed: cannot locate symbol "napi_create_function"
Verified on an android-arm64 emulator for the present-but-broken case, and on
the host for the missing and corrupt cases.
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.
Both changes came out of debugging a rocksdb-native prebuild run that failed with
Cannot find addonwhile the file was sitting on the device the whole time. Neither changes what passes or fails — they only make a failing run explain itself."Cannot find addon" when the addon is right there
require-addontries torequire()every candidate and raisesCannot find addonwhen they all fail, so a prebuild that exists but can'tdlopenreports identically to one that was never installed. Both runners printed onlyerr.stack, which omitscause.Printing
causeisn't enough: the resolver assigns it per candidate, so it ends up holding the last candidate's error — normally aCannot find modulefor a root-level path that was never going to exist. The actual dlopen error gets overwritten.The error does carry
candidates, so this retries the ones that exist on disk and reports why each failed. The rocksdb-native run now says:which is the answer (holepunch's npm android
.nodelinks nolibnode.so), rather than a wrong hint to go looking for a missing file. Applied todefault-smoke-test.jstoo, since smoke is the default runner and this is exactly the failure it exists to catch.Failures logged under tags the runner silenced
run-test.shstreamed onlyNODEJS-MOBILE(node's stdout/stderr, pumped bynative-lib.cpp). Node's fatal-error reports go to tagnodejsinstead, andTestActivity's asset copy printStackTraces per-file failures toSystem.err— both dropped, so a broken run looked like silence. FindingError: ENOENT ... open 'test/fixtures/lock'needed unfiltered logcat on a local emulator. Addsnodejs,System.err,AndroidRuntime; keeps-v rawso TAP output and sentinel parsing are unchanged.Verification
node --check,bash -n, shellcheck cleanNot included: the
APPLE_CLANG=ONedit sitting uncommitted in my working copy ofprebuild/action.yml— that's a separate change and still unreleased, so iOS prebuilds built fromv2don't have it.