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.

Sign up for GitHub

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

Jump to bottom

kill debug assert and unnecessary macros #3556

Merged
merged 6 commits into from
Aug 13, 2019
Merged

kill debug assert and unnecessary macros #3556

merged 6 commits into from
Aug 13, 2019

Conversation

charlotteliang
Copy link
Contributor

@charlotteliang charlotteliang commented Aug 12, 2019 •
edited
Loading

Remove _FIRMessagingDevAssert that are only called if a DEBUG flag, which is never set in both debug/production build. Most of the assert logics are already been checked in the code, for the ones that are not, we replace it with a debugLog.
Also remove a few macros that are either not used at all or barely used.
@charlotteliang charlotteliang changed the title kill debug code Aug 12, 2019
morganchen12
morganchen12 reviewed Aug 12, 2019
View reviewed changes
Copy link
Contributor

@morganchen12 morganchen12 left a comment

Choose a reason for hiding this comment

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

Consider replacing all or some of these asserts with warning logs.
morganchen12
morganchen12 approved these changes Aug 12, 2019
View reviewed changes
Copy link
Contributor

@morganchen12 morganchen12 left a comment

Choose a reason for hiding this comment

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

LGTM, but please wait for core approval before merging.
@charlotteliang
Copy link
Contributor Author

Good catch! I'm adding more debug/warning logs.
@paulb777
Copy link
Member

Looks like real failures in travis:

Failing tests:
	-[FIRMessagingServiceTest testSubscribeWithInvalidTopic]
	-[FIRMessagingServiceTest testSubscribeWithInvalidTopic]
	-[FIRMessagingServiceTest testSubscribeWithInvalidTopic]
	-[FIRMessagingServiceTest testUnsubscribeWithInvalidTopic]
	-[FIRMessagingServiceTest testUnsubscribeWithInvalidTopic]
	-[FIRMessagingServiceTest testUnsubscribeWithInvalidTopic]
@charlotteliang charlotteliang merged commit eade6bd into master Aug 13, 2019
@charlotteliang charlotteliang deleted the fcm-kill-devAssert branch August 13, 2019 21:12
@paulb777 paulb777 mentioned this pull request Aug 14, 2019
Merged
@firebase firebase locked and limited conversation to collaborators Oct 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes
4 participants
@charlotteliang @paulb777 @morganchen12 @googlebot