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
Refactor and add ut for ClassUtils #4887
Conversation
externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/spark/ui/EngineTab.scala
Outdated
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## master #4887 +/- ##
======================================
Coverage 0.00% 0.00%
======================================
Files 557 557
Lines 30705 30694 -11
Branches 3997 3993 -4
======================================
+ Misses 30705 30694 -11
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/spark/ui/EngineTab.scala
Outdated
Show resolved
Hide resolved
externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/spark/ui/EngineTab.scala
Outdated
Show resolved
Hide resolved
Can I move the |
It's fine if other places can benefit from it, but since Java does not support the default value, it should be broken into multi-methods |
OK, let's leave it in Scala module for now. As for Java, it's handy to use workaround |
Thanks, merged to master. |
### _Why are the changes needed?_ - add unit tests for `ClassUtils` - refactor `ClassUtils.createInstance` method with DynConstructors - move `classIsLoadable` method to `ReflectUtils.isClassLoadable`, refeactor to use DynClasses ### _How was this patch tested?_ - [ ] Add some test cases that check the changes thoroughly including negative and positive cases if possible - [ ] Add screenshots for manual tests if appropriate - [x] [Run test](https://kyuubi.readthedocs.io/en/master/develop_tools/testing.html#running-tests) locally before make a pull request Closes apache#4887 from bowenliang123/classutil. Closes apache#4887 c39f9a0 [liangbowen] simplify ut 633e21d [liangbowen] Refactor and add ut for ClassUtils Authored-by: liangbowen <liangbowen@gf.com.cn> Signed-off-by: liangbowen <liangbowen@gf.com.cn>
Why are the changes needed?
ClassUtils
ClassUtils.createInstance
method with DynConstructorsclassIsLoadable
method toReflectUtils.isClassLoadable
, refeactor to use DynClassesHow was this patch tested?
Add some test cases that check the changes thoroughly including negative and positive cases if possible
Add screenshots for manual tests if appropriate
Run test locally before make a pull request