-
Notifications
You must be signed in to change notification settings - Fork 46
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
Add internal testing tools package #587
Add internal testing tools package #587
Conversation
175600d
to
28ef98c
Compare
969d9bb
to
b6ce681
Compare
3f2857e
to
de6a7d5
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.
It looks great on the iOS side, well done!
I have left a coupe of suggestions.
packages/internal-testing-tools/ios/Sources/DatadogCoreProxy.swift
Outdated
Show resolved
Hide resolved
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.
nice work! I added few comments/thoughts.
packages/core/android/src/main/kotlin/com/datadog/reactnative/DatadogSDKWrapper.kt
Outdated
Show resolved
Hide resolved
packages/core/android/src/main/kotlin/com/datadog/reactnative/DatadogSDKWrapper.kt
Outdated
Show resolved
Hide resolved
packages/core/android/src/main/kotlin/com/datadog/reactnative/DatadogSDKWrapper.kt
Outdated
Show resolved
Hide resolved
packages/core/android/src/main/kotlin/com/datadog/reactnative/DatadogSDKWrapper.kt
Outdated
Show resolved
Hide resolved
/** | ||
* Internal object used to add internal testing. | ||
*/ | ||
object DatadogSDKWrapperStorage { |
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.
If we don't want to make it available to the customers, we need to make this class internal
.
object DatadogSDKWrapperStorage { | |
internal object DatadogSDKWrapperStorage { |
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.
If I make it internal
, I cannot import it in the internal testing module.
Is it possible to still import it?
And I think it can be ok to leave it available to customers. Most of them won't do anything, and maybe that can make things a little easier for some customers with hybrid applications.
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.
Let's keep it public then, but if it used only for the tests, nothing in the name reflects that the intended usage is for the tests only.
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.
It will also be used in Session Replay to make sure we use the same core for all features - it's one of the next tasks on the topic.
When we'll split the RN SDK in core/RUM/Logs/Trace this will also be used to get the core in the feature packages.
Let me know if you think we should still change the name to something more explicit here.
...src/test/kotlin/com/datadog/reactnative/internaltesting/DdSessionReplayImplementationTest.kt
Outdated
Show resolved
Hide resolved
...src/test/kotlin/com/datadog/reactnative/internaltesting/DdSessionReplayImplementationTest.kt
Show resolved
Hide resolved
mockContext | ||
) | ||
DatadogSDKWrapperStorage.setSdkCore(mockCore) | ||
DatadogSDKWrapperStorage.notifyOnInitializedListeners() |
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.
So this call will do another setSdkCore
call, but with StubSDKCore
this time? It is a bit non-intuitive by looking on the class name. Maybe we need some method there which has something related to the stubbing directly in the name?
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.
We do not do another setSdkCore
call directly, but we notify all listeners, and one of them happens to be setting the SdkCore.
I don't think this is a good idea to reflect in the name of notifyOnInitializedListeners
that it's currently only being used for internal testing since it is in the main SDK package and we might as well add other callbacks in the future.
I can add a comment in the test and above the function to make it clearer, would that work for you?
...src/test/kotlin/com/datadog/reactnative/internaltesting/DdSessionReplayImplementationTest.kt
Outdated
Show resolved
Hide resolved
...src/test/kotlin/com/datadog/reactnative/internaltesting/DdSessionReplayImplementationTest.kt
Outdated
Show resolved
Hide resolved
de6a7d5
to
9288937
Compare
9a317d9
to
14f3325
Compare
14f3325
to
66855bb
Compare
What does this PR do?
This adds a new package for internal UI-level testing.
The package has 3 methods:
enable
(to be called before initializing the SDK): registers hooks to capture eventsgetAllEvents
: returns all captured events for a given featureclearData
: removes all captured eventsMotivation
Enable UI-level testing for the SDK.
Review checklist (to be filled by reviewers)