-
Notifications
You must be signed in to change notification settings - Fork 277
feat: feature specific tests #2372
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
Conversation
5448e50 to
a8c1da3
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2372 +/- ##
============================================
+ Coverage 56.12% 57.43% +1.30%
- Complexity 976 1296 +320
============================================
Files 119 147 +28
Lines 11743 13418 +1675
Branches 2251 2349 +98
============================================
+ Hits 6591 7706 +1115
- Misses 4012 4449 +437
- Partials 1140 1263 +123 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@wForget ptal |
| test("test native_datafusion scan on fake fs") { | ||
| // Skip test if HDFS feature is not enabled in native library | ||
| val hdfsEnabled = | ||
| try { |
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.
Should we extract this try...catch block into a common method in NativeBase?
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.
Good idea. Done
| /// * `1` (true) if the feature is enabled | ||
| /// * `0` (false) if the feature is disabled or unknown | ||
| #[no_mangle] | ||
| pub extern "system" fn Java_org_apache_comet_NativeBase_isFeatureEnabled( |
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.
@parthchandra please help me to understand where the feature comes from?
Another thing would it support multiple features like https://doc.rust-lang.org/cargo/reference/features.html#command-line-feature-options
features=f1,f2 ?
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.
The name of the feature comes from the features specified in cargo.toml. And no, this is one feature at a time.
|
How about we replace it with |
Um. It's not that expensive to make a jni call in a unit test :) |
Co-authored-by: Oleks V <comphead@users.noreply.github.com>
comphead
left a comment
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.
lgtm thanks @parthchandra
* feat: feature specific tests
Which issue does this PR close?
Closes #2360.
Rationale for this change
Some tests should be run only when the binary has been built with specific features
What changes are included in this PR?
Adds a jni call to check if a feature required by the test is available in the build
How are these changes tested?