Fix: Breadcrumbs added on forked context are now captured #629
Conversation
lucas-zimerman
changed the title
…version. Added tests. used a better name for the new function that joins two groups of breadcrumbs. Removed comment from java layer. Removed duplicated tests on the wrapper test file
lucas-zimerman
changed the title
| * @param nativeList The second group of breadcrumbs from the native layer. | ||
| * @returns An array of unique breadcrumbs merged from both lists. | ||
| */ | ||
| private _mergeUniqueBreadcrumbs(jsList: Breadcrumb[], nativeList: Breadcrumb[]): Breadcrumb[] { |
There was a problem hiding this comment.
Thank you for adding test for this method, it makes it super easy to understand how it would behave.
m: I'm not sure if we can be sure that the breadcrumbs are in the same order in both arrays.
If I'm looking at the correctly in case they would not be in the same order the duplicates would not be removed. Creating possible double the size.
We should also respect the maxBreadcrumbs size.
| } | ||
| }); | ||
|
|
||
| describe('Device Context Breadcrumb filter', () => { |
There was a problem hiding this comment.
I think test like this would not pass, but from the code I understand that the breadcrumbs should always be ordered the same in both arrays?
I think we should add this test so it's clear in the future we were aware of what happens in this situation. And also keep maxBreadcrumbs.
const jsList = [
{ timestamp: 2, message: 'new js' },
{ timestamp: 1, message: 'new js' },
{ timestamp: 3, message: 'new js' },
{ timestamp: 4, message: 'new js' },
] as Breadcrumb[];
const nativeList = [
{ timestamp: 2, message: 'new native' },
{ timestamp: 1, message: 'new js' },
{ timestamp: 2, message: 'new js' },
{ timestamp: 3, message: 'new native' },
{ timestamp: 4, message: 'new native' },
] as Breadcrumb[];| } | ||
| }); | ||
|
|
||
| describe('Device Context Breadcrumb filter', () => { |
There was a problem hiding this comment.
We should add test for user set timeout. Something like this:
const jsList = [
{ timestamp: 2, message: 'new js' },
{ timestamp: 1, message: 'new js' },
] as Breadcrumb[];
const nativeList = [
{ timestamp: 2, message: 'new native' },
{ timestamp: 2, message: 'new js' },
{ timestamp: 1, message: 'new js' },
{ timestamp: 3, message: 'new native' },
] as Breadcrumb[];
const expected = [
{ timestamp: 2, message: 'new native' },
{ timestamp: 2, message: 'new js' },
{ timestamp: 1, message: 'new js' },
{ timestamp: 3, message: 'new native' },
] as Breadcrumb[];| if (!this.isNativeClientAvailable()) { | ||
| throw this._NativeClientError; | ||
| } |
| @@ -316,22 +310,9 @@ export const NATIVE = { | |||
| // @ts-ignore Android still uses the old message object, without this the serialization of events will break. | |||
| event.message = { message: event.message }; | |||
There was a problem hiding this comment.
This should still be execute only for Android.
|
This pull request has gone three weeks without activity. In another week, I will close it. But! If you comment or otherwise update it, I will reset the clock, and if you label it "A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀 |
This PR brings more native context data to be synced with captured events and also allows hint breadcrumbs to be properly registered, for example:
will now be registered on the event, including the native events:
Additional notes on the PR:
Closes #263