-
Notifications
You must be signed in to change notification settings - Fork 987
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
Don't parse unused identicon in android pn #17174
Conversation
|
||
String name = bundle.getString("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.
looks good! tiny nitpick on placing a TODO comment or link to issue here :)
Jenkins BuildsClick to see older builds (18)
|
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
I am trying to set up unit tests for this part, but getting into some issues
Just out of curiosity, which problems @cammellos? Or better yet, what were you trying to test?
I am trying to test the native java code we use for push notifications, ie this file:
This is mocked in our unit tests, and only tested through e2e tests. Problems have mostly to do with dependencies etc, but I have solved them now, though I am still working through mocking/etc in the actual test |
👍🏼 Cool, definitely valuable to have unit test coverage there. |
4f2714b
to
e6e3d9a
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.
🚀
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's excellent to see the implementation of unit tests for relevant Android native code. This is a great initiative, and I'm looking forward to delving deeper into this aspect.
I didn't find any major issues with the code. LGTM 👍
Hi @cammellos : I really like this attempt of adding tests to native android code! I see you've added some This PR will require those 3 files will that get auto generated :
|
e6e3d9a
to
78f1207
Compare
a948335
to
9b4ea2b
Compare
84% of end-end tests have passed
Failed tests (7)Click to expandClass TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMergedTwo:
Passed tests (36)Click to expandClass TestActivityMultipleDevicePRTwo:
Class TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMergedTwo:
|
79% of end-end tests have passed
Failed tests (9)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Passed tests (34)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityOneDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestActivityMultipleDevicePR:
Class TestActivityMultipleDevicePRTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
|
@cammellos Thank you for the fix, awesome work! |
9b4ea2b
to
155f57c
Compare
Removes parsing of identicon from notification, which won't be showing it anymore (it's a bit more complicated to add the actual image, so removing for now).
I am trying to set up unit tests for this part, but getting into some issues, as this is a blocker, I will send the PR for review now
Fixes: #17160