-
Notifications
You must be signed in to change notification settings - Fork 2
Run test app on CI #133
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Run test app on CI #133
Conversation
|
d992ba6
to
55df470
Compare
55df470
to
caa8fe8
Compare
a994bd8
to
6f21e22
Compare
6f21e22
to
09f89d6
Compare
09f89d6
to
5c74bb9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR updates the CI check workflow to run the React Native test app on labeled Android/iOS runners and integrates mocha-remote
for end-to-end tests in the app.
- Adds
test:android
andtest:ios
scripts inapps/test-app
usingmocha-remote
+concurrently
- Refactors
App.tsx
to useMochaRemoteProvider
/signals for test runner UI - Extends CI workflow (
.github/workflows/check.yml
) with newtest-ios
andtest-android
jobs conditional on labels
Reviewed Changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
packages/node-addon-examples/index.js | Temporarily commented out a crashing example |
apps/test-app/package.json | Added Metro and test scripts, new test dependencies |
apps/test-app/App.tsx | Switched to MochaRemoteProvider and signal-based UI |
.github/workflows/check.yml | New CI jobs: iOS/Android test app runs based on PR labels |
Comments suppressed due to low confidence (4)
.github/workflows/check.yml:71
- [nitpick] The iOS test job installs Android SDK and NDK – consider removing this step from the iOS workflow to reduce setup time and eliminate irrelevant setup.
- name: Setup Android SDK
apps/test-app/package.json:10
- The
mocha-remote
flag is written as-- concurrently
but should be--concurrently
(no space) to correctly enable concurrent execution.
"test:android": "mocha-remote --exit-on-error -- concurrently --kill-others-on-fail --passthrough-arguments npm:metro 'npm:android -- {@}' --",
apps/test-app/package.json:11
- Likewise, change
-- concurrently
to--concurrently
so themocha-remote
script picks up the concurrently flag properly.
"test:ios": "mocha-remote --exit-on-error -- concurrently --passthrough-arguments --kill-others-on-fail npm:metro 'npm:ios -- {@}' --"
apps/test-app/App.tsx:11
- The import path
@react-native-node-api/node-addon-examples
does not match the dependency name (react-native-node-addon-examples
) in package.json; update one to match the other.
import nodeAddonExamples from "@react-native-node-api/node-addon-examples";
// TODO: This crashes (SIGABRT) | ||
"async_work_thread_safe_function": () => require("./examples/5-async-work/async_work_thread_safe_function/napi/index.js"), | ||
// "async_work_thread_safe_function": () => require("./examples/5-async-work/async_work_thread_safe_function/napi/index.js"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Consider removing this long-commented-out example or extracting it behind a feature flag to avoid confusion and clean up the code.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very thorough!
- name: Clone patched Hermes version | ||
shell: bash | ||
run: | | ||
REACT_NATIVE_OVERRIDE_HERMES_DIR=$(npx react-native-node-api vendor-hermes --silent) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could being silent here hide any warnings that are early signs of issues?
uses: android-actions/setup-android@v3 | ||
with: | ||
packages: tools platform-tools ndk;${{ env.NDK_VERSION }} | ||
- run: rustup target add x86_64-linux-android aarch64-linux-android armv7-linux-androideabi i686-linux-android aarch64-apple-ios-sim |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm guessing rustup just skips the nonsensical toolchain combinations (ios-sim on windows, etc)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure you can cross-compile any of these. Or at the very least install the targets on any system.
Merging this PR will update the check workflow on CI to run the test app:
Also ...
I suggest using mocha-remote (a tool I maintain and use for Realm JS) to drive the "integrated unit tests", at least for now. I'm open to switching this out for another "universal testing library".