-
Notifications
You must be signed in to change notification settings - Fork 222
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 app install, foreground and background event and application entity tracking #1396
Add app install, foreground and background event and application entity tracking #1396
Conversation
BundleMonFiles added (6)
Total files change +105.95KB 0% Final result: ✅ View report in BundleMon website ➡️ |
tracker.track(buildPageView({ pageUrl: 'http://localhost' })); | ||
|
||
expect(payloads.length).toBe(1); | ||
expect((payloads[0]?.co as string) ?? '').not.toContain(MOBILE_APPLICATION_CONTEXT_SCHEMA); |
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.
nit: Using fallback values for the values we are testing feels a bit off. Maybe something like expect(payloads[0]?.co).toBeUndefined();
could be better? co
should never be populated here, so that would catch a case where there is somehow (cosmic ray, act of god, who knows) a value for co
that doesn't match one of those schemas.
Or, if we want to still explicitly check for those schemas, the checks could be split like so?:
if (payloads[0]?.co) {
expect(payloads[0]?.co).not.toContain(MOBILE_APPLICATION_CONTEXT_SCHEMA);
expect(payloads[0]?.co).not.toContain(APPLICATION_CONTEXT_SCHEMA);
} else {
expect(payloads[0]?.co).toBeUndefined();
}
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.
Sorry, I'm not totally following. The snippet that you shared is doing the same thing as the code above? The only difference I can see is in handling the empty string value – your snippet will raise an error in that case. But I don't think that's something we should be testing at this point, an empty string in the co
field would be a problem at a different level (in the tracker core).
co
should never be populated here
I didn't want to make this assumption. We might extend the tracker in the future to add more autotracked context entities and I didn't want the test to break in case we do so. Maybe that's not very likely especially since I'm using the tracker core, not the RN tracker here, but I have had to rework unit tests so many times when an unrelated change happens that I'm wary to test more than what the test case actually is supposed to test.
3376da0
to
56eb447
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.
lgtm
Adds 3 internal plugins to the RN tracker: