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

Crash preventation #4661

Merged
merged 1 commit into from
Jan 27, 2020
Merged

Crash preventation #4661

merged 1 commit into from
Jan 27, 2020

Conversation

GUserx
Copy link
Contributor

@GUserx GUserx commented Jan 11, 2020 •
edited by paulb777
Loading

Fixes #4659
@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.
@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.
@GUserx GUserx mentioned this pull request Jan 11, 2020
Closed
GUserx
GUserx commented Jan 12, 2020
View reviewed changes
Crashlytics/Crashlytics/Models/FIRCLSFileManager.m Outdated
Comment on lines 309 to 316
BOOL contentsOfProcessingPathRemoved = [self removeContentsOfProcessingPath];
BOOL contentsOfPendingPathRemoved = [self removeContentsOfPendingPath];
BOOL contentsOfDirectoryAtPreparedPathRemoved =
[self removeContentsOfDirectoryAtPath:self.preparedPath];
BOOL contentsOfDirectoryAtActivePathRemoved =
[self removeContentsOfDirectoryAtPath:self.activePath];
success = contentsOfProcessingPathRemoved && contentsOfPendingPathRemoved &&
contentsOfDirectoryAtPreparedPathRemoved && contentsOfDirectoryAtActivePathRemoved;
Copy link
Contributor Author

@GUserx GUserx Jan 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Here might be this type of form if you wish:
return [self removeContentsOfProcessingPath] & [self removeContentsOfPendingPath] & ... etc
But the above case is more friendly for debug imo =)
Copy link
Contributor

Choose a reason for hiding this comment

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

We can declare success at the time we check for all the criteria:
BOOL success = contentsOfProcessingPathRemoved && contentsOfPendingPathRemoved &&
contentsOfDirectoryAtPreparedPathRemoved && contentsOfDirectoryAtActivePathRemoved;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can declare success at the time we check for all the criteria:
BOOL success = contentsOfProcessingPathRemoved && contentsOfPendingPathRemoved &&
contentsOfDirectoryAtPreparedPathRemoved && contentsOfDirectoryAtActivePathRemoved;

You are definitely right :)
Thanks!
@GUserx GUserx closed this Jan 12, 2020
@GUserx GUserx reopened this Jan 12, 2020
@paulb777
Copy link
Member

@GUserx Thanks for the suggestions.

We'd appreciate if you'd send separate PRs for different pods. In this case, a separate PR for the Installations versus Crashylytics would help with the review process.

For the Installations change, please investigate the subsequent CI failure in https://travis-ci.org/firebase/firebase-ios-sdk/jobs/635952938

- NOTE  | [iOS] xcodebuild:  /Users/travis/build/firebase/firebase-ios-sdk/Example/Messaging/Tests/FIRMessagingContextManagerServiceTest.m:85: error: -[FIRMessagingContextManagerServiceTest testMessageWithFutureStartTime] : ((self.scheduledLocalNotifications.count) equal to (1)) failed: ("0") is not equal to ("1")
- NOTE  | [iOS] xcodebuild:  /Users/travis/build/firebase/firebase-ios-sdk/Example/Messaging/Tests/FIRMessagingContextManagerServiceTest.m:170: error: -[FIRMessagingContextManagerServiceTest testTimedNotificationsUserInfo] : ((self.scheduledLocalNotifications.count) equal to (1)) failed: ("0") is not equal to ("1")
- NOTE  | [iOS] xcodebuild:  /Users/travis/build/firebase/firebase-ios-sdk/Example/Messaging/Tests/FIRMessagingContextManagerServiceTest.m:175: error: -[FIRMessagingContextManagerServiceTest testTimedNotificationsUserInfo] : ((notification.userInfo[messageIdentifierKey]) equal to (messageIdentifier)) failed: ("(null)") is not equal to ("fcm-cm-test1")
- NOTE  | [iOS] xcodebuild:  /Users/travis/build/firebase/firebase-ios-sdk/Example/Messaging/Tests/FIRMessagingContextManagerServiceTest.m:176: error: -[FIRMessagingContextManagerServiceTest testTimedNotificationsUserInfo] : ((notification.userInfo[customDataKey]) equal to (customData)) failed: ("(null)") is not equal to ("world")
@GUserx
Copy link
Contributor Author

GUserx commented Jan 12, 2020

@GUserx Thanks for the suggestions.

We'd appreciate if you'd send separate PRs for different pods. In this case, a separate PR for the Installations versus Crashylytics would help with the review process.

For the Installations change, please investigate the subsequent CI failure in https://travis-ci.org/firebase/firebase-ios-sdk/jobs/635952938

- NOTE  | [iOS] xcodebuild:  /Users/travis/build/firebase/firebase-ios-sdk/Example/Messaging/Tests/FIRMessagingContextManagerServiceTest.m:85: error: -[FIRMessagingContextManagerServiceTest testMessageWithFutureStartTime] : ((self.scheduledLocalNotifications.count) equal to (1)) failed: ("0") is not equal to ("1")
- NOTE  | [iOS] xcodebuild:  /Users/travis/build/firebase/firebase-ios-sdk/Example/Messaging/Tests/FIRMessagingContextManagerServiceTest.m:170: error: -[FIRMessagingContextManagerServiceTest testTimedNotificationsUserInfo] : ((self.scheduledLocalNotifications.count) equal to (1)) failed: ("0") is not equal to ("1")
- NOTE  | [iOS] xcodebuild:  /Users/travis/build/firebase/firebase-ios-sdk/Example/Messaging/Tests/FIRMessagingContextManagerServiceTest.m:175: error: -[FIRMessagingContextManagerServiceTest testTimedNotificationsUserInfo] : ((notification.userInfo[messageIdentifierKey]) equal to (messageIdentifier)) failed: ("(null)") is not equal to ("fcm-cm-test1")
- NOTE  | [iOS] xcodebuild:  /Users/travis/build/firebase/firebase-ios-sdk/Example/Messaging/Tests/FIRMessagingContextManagerServiceTest.m:176: error: -[FIRMessagingContextManagerServiceTest testTimedNotificationsUserInfo] : ((notification.userInfo[customDataKey]) equal to (customData)) failed: ("(null)") is not equal to ("world")

@paulb777 Thanks for your comment.
I did revert to previous version of this commit.
@paulb777
Copy link
Member

paulb777 commented Jan 12, 2020 •
edited
Loading

Thanks. Note that CI did not run for Crashlytics because of the issue fixed by #4663
@samedson
Copy link
Contributor

@GUserx thanks for the PR! We're going to run through some validation and review this PR later in the week after we've gone through with the first release
@GUserx
Copy link
Contributor Author

GUserx commented Jan 13, 2020

@GUserx thanks for the PR! We're going to run through some validation and review this PR later in the week after we've gone through with the first release

Got it. Thank you for info.
@morganchen12 morganchen12 requested review from samedson and removed request for samedson January 21, 2020 20:53
@jasonhu-g jasonhu-g merged commit 34f720c into firebase:master Jan 27, 2020
@samedson samedson mentioned this pull request Jan 31, 2020
Merged
@firebase firebase locked and limited conversation to collaborators Feb 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api: crashlytics cla: yes
5 participants
@GUserx @googlebot @paulb777 @samedson @jasonhu-g