Skip to content
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

convert ReactPropAnnotationSetterTest to Kotlin #39706

Closed

Conversation

huzhanbo1996
Copy link
Contributor

Summary:

fix and convert ReactPropAnnotationSetterTest to Kotlin for #38825

Changelog:

[ANDROID] [CHANGED] - Rewrite ReactPropAnnotationSetterTest to Kotlin,

Test Plan:

yarn && ./gradlew :packages:react-native:ReactAndroid:test

@cortinico @mdvacca

@facebook-github-bot
Copy link
Contributor

Hi @huzhanbo1996!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

@github-actions
Copy link

github-actions bot commented Sep 28, 2023

Warnings
⚠️ One hour and a half have passed and the E2E jobs haven't finished yet.

Generated by 🚫 dangerJS against 842fa4c

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Sep 28, 2023
@analysis-bot
Copy link

analysis-bot commented Sep 28, 2023

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,342,700 -8,560,218
android hermes armeabi-v7a n/a --
android hermes x86 n/a --
android hermes x86_64 n/a --
android jsc arm64-v8a 9,580,803 -10,800,862
android jsc armeabi-v7a n/a --
android jsc x86 n/a --
android jsc x86_64 n/a --

Base commit: 6635474
Branch: main

Copy link
Contributor

@cortinico cortinico left a comment

Choose a reason for hiding this comment

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

Thanks for sending this over @huzhanbo1996
There are a couple of changes that we need to apply before we can merge this


@Suppress("UNUSED_PARAMETER")
private inner class ViewManagerUnderTest
constructor(val mViewManagerUpdatesReceiver: ViewManagerUpdatesReceiver) :
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
constructor(val mViewManagerUpdatesReceiver: ViewManagerUpdatesReceiver) :
constructor(val viewManagerUpdatesReceiver: ViewManagerUpdatesReceiver) :

@Suppress("UNUSED_PARAMETER")
private inner class ViewManagerUnderTest
constructor(val mViewManagerUpdatesReceiver: ViewManagerUpdatesReceiver) :
ViewManager<View?, ReactShadowNode<*>?>() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ViewManager<View?, ReactShadowNode<*>?>() {
ViewManager<View, ReactShadowNode<*>>() {

Comment on lines 58 to 60
override fun getName(): String {
return "RedpandasLivestreamVideoView"
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
override fun getName(): String {
return "RedpandasLivestreamVideoView"
}
override fun getName() = "RedpandasLivestreamVideoView"

Comment on lines 62 to 65
override fun createShadowNodeInstance(): ReactShadowNode<*>? {
Assertions.fail<Any>("This method should not be executed as a part of this test")
return null
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
override fun createShadowNodeInstance(): ReactShadowNode<*>? {
Assertions.fail<Any>("This method should not be executed as a part of this test")
return null
}
override fun createShadowNodeInstance(): ReactShadowNode<*>? =
error("This method should not be executed as a part of this test")

Comment on lines 177 to 179
private var mViewManager: ViewManagerUnderTest? = null
private var mUpdatesReceiverMock: ViewManagerUpdatesReceiver? = null
private var mTargetView: View? = null
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
private var mViewManager: ViewManagerUnderTest? = null
private var mUpdatesReceiverMock: ViewManagerUpdatesReceiver? = null
private var mTargetView: View? = null
private lateinit var viewManager: ViewManagerUnderTest
private lateinit var updatesReceiverMock: ViewManagerUpdatesReceiver
private lateinit var targetView: View

Copy link
Contributor

Choose a reason for hiding this comment

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

Please also update all the !! in this test file because of this change

Comment on lines 446 to 448
fun buildStyles(vararg keysAndValues: Any?): ReactStylesDiffMap {
return ReactStylesDiffMap(JavaOnlyMap.of(*keysAndValues))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
fun buildStyles(vararg keysAndValues: Any?): ReactStylesDiffMap {
return ReactStylesDiffMap(JavaOnlyMap.of(*keysAndValues))
}
fun buildStyles(vararg keysAndValues: Any?) =
ReactStylesDiffMap(JavaOnlyMap.of(*keysAndValues))

Comment on lines 67 to 69
override fun getShadowNodeClass(): Class<out ReactShadowNode<*>> {
return ReactShadowNode::class.java
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Please update this and all the other getters to return an expression

Comment on lines 71 to 74
override fun createViewInstance(reactContext: ThemedReactContext): View {
Assertions.fail<Any>("This method should not be executed as a part of this test")
return View(RuntimeEnvironment.getApplication())
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
override fun createViewInstance(reactContext: ThemedReactContext): View {
Assertions.fail<Any>("This method should not be executed as a part of this test")
return View(RuntimeEnvironment.getApplication())
}
override fun createViewInstance(reactContext: ThemedReactContext): View =
error("This method should not be executed as a part of this test")

Please apply the same change to all the similar methods that we don't expect to call inside the tests

@huzhanbo1996
Copy link
Contributor Author

Thanks for the suggestions, please take a look again at the changes and the reply @cortinico

@facebook-github-bot
Copy link
Contributor

@cortinico has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label Oct 3, 2023
@facebook-github-bot
Copy link
Contributor

@cortinico merged this pull request in ea88338.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants