Skip to content

Assert logs and run test suites conditionally #143

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Jul 23, 2025
Merged

Assert logs and run test suites conditionally #143

merged 4 commits into from
Jul 23, 2025

Conversation

kraenhansen
Copy link
Collaborator

@kraenhansen kraenhansen commented Jun 25, 2025 •
edited
Loading

Merging this PR will:

  • Assert log output from Node Addon examples
  • Run tests conditionally based on Mocha Remote context

This also fixes #141.

@kraenhansen kraenhansen self-assigned this Jun 25, 2025
@kraenhansen kraenhansen added CI Continuous integration Apple 🍎 Anything related to the Apple platform (iOS, macOS, Cocoapods, Xcode, XCFrameworks, etc.) Android 🤖 Anything related to the Android platform (Gradle, NDK, Android SDK) labels Jun 25, 2025
@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Jun 25, 2025 •
edited
Loading

⚠️ No Changeset found

Latest commit: 34408c3

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@kraenhansen kraenhansen changed the title Assert log output from Node Addon examples and run tests conditionally based on Mocha Remote context Assert logs and run test suites conditionally Jun 25, 2025
@kraenhansen kraenhansen force-pushed the kh/assert-example-logs branch from 88a424b to 6ca4b54 Compare June 28, 2025 18:48
@kraenhansen kraenhansen force-pushed the main branch 4 times, most recently from 90dd405 to 8dac805 Compare July 7, 2025 12:30
@kraenhansen kraenhansen force-pushed the kh/assert-example-logs branch from 6ca4b54 to 60677f4 Compare July 22, 2025 21:44
shirakaba
shirakaba approved these changes Jul 22, 2025
View reviewed changes
Copy link
Collaborator

@shirakaba shirakaba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor comments!

packages/node-addon-examples/src/index.ts Outdated
}
}

export const suites: Record<string, Record<string, () => void>> = {
Copy link
Collaborator

@shirakaba shirakaba Jul 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't clear how/when this test file would be run, nor whether it'd be in Node.js or React Native. May be worth adding some brief docs in packages/node-addon-examples/README.md and apps/test-app/README.md.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I revisited the packages/node-addon-examples/README.md.

apps/test-app/App.tsx
Comment on lines +28 to +30
allTests = false,
nodeAddonExamples = allTests,
ferricExample = allTests,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So by default, both nodeAddonExamples and ferricExample resolve to false and so both the "Node Addon Examples" and "ferric-example" tests get skipped? Is that the intention?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might still be debating this with myself 😊

On one hand it might be confusing to skip so many tests by default and on the other it does leave a potential to cut down on the time pre-building while bootstrapping and complexity (having to have the Rust toolchain installed) when someone wants to contribute to the project.

Looking back at this (I wrote it some weeks back), the optimization does seem a bit premature.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my (though not strongly held)) opinion we could either enable all tests by default or skip only ferric-example since it requires additional Rust setup.

@kraenhansen kraenhansen force-pushed the kh/assert-example-logs branch from 60677f4 to 8afc47e Compare July 23, 2025 06:54
@paradowstack
Copy link
Contributor

Great job, I liked introduced changes and converting example into real tests.
It also gets us a bit closer to support node-api tests. However this step will be more complicated, because these tests rely heavily on node modules that we will have to mock or polyfill (I can try to accomplish that when implementation of Node-API V1 runtime functions is completed (tested so far by our own adaptation of these tests).

@shirakaba
Copy link
Collaborator

Feel free to merge if you're happy – I am unlikely to have time to look (and understand) much deeper!

@kraenhansen kraenhansen force-pushed the kh/assert-example-logs branch from 8afc47e to 34408c3 Compare July 23, 2025 12:10
@kraenhansen kraenhansen merged commit 2b7644f into main Jul 23, 2025
6 checks passed
@kraenhansen kraenhansen deleted the kh/assert-example-logs branch July 23, 2025 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shirakaba shirakaba shirakaba approved these changes

@paradowstack paradowstack paradowstack approved these changes

Assignees

@kraenhansen kraenhansen

Labels
Android 🤖 Anything related to the Android platform (Gradle, NDK, Android SDK) Apple 🍎 Anything related to the Apple platform (iOS, macOS, Cocoapods, Xcode, XCFrameworks, etc.) CI Continuous integration
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Enable running suites in the test app conditionally
3 participants
@kraenhansen @paradowstack @shirakaba