-
Notifications
You must be signed in to change notification settings - Fork 486
DatabaseUI Tests: Road to green CI #108
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
Conversation
96b64f4
to
86e06ee
Compare
|
||
/* Begin PBXShellScriptBuildPhase section */ | ||
072ABD579ED6CE7C8FC98658 /* 📦 Check Pods Manifest.lock */ = { | ||
072ABD579ED6CE7C8FC98658 /* [CP] Check Pods Manifest.lock */ = { |
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.
Deintegrate
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.
pod-deintegrate doesn't delete those. You can simply delete those 3 build phases in xcode.
General CL looks good - definitely add a small changes/migration list, and any updates to docs. One thing that occurs to me is that the fakes you made for the database primitives may be generally useful for developers. It might be worth considering cleaning them up and moving into a separate subproject so that people can use them in their own tests. |
@ianbarber this PR now includes tests for basically all of databaseUI, which imo is good enough to merge and try to get CI up and running. Take a look at this when you can and I'll eventually put together a comprehensive changelist before merging. Thanks! |
22bf0f8
to
577a3d7
Compare
Changelog: FirebaseUI Changes
This only includes breaking and non-breaking API changes, not bug fixes. |
if (self) { | ||
_clientID = [clientID copy]; | ||
_scopes = @[ @"email", @"profile" ]; | ||
_scopes = scopes; |
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.
This is a bit of an example of the concern from before - you're assigning to scopes, and skipping the "copy" property attribute. I think the right thing to do is to make the public property nonatomic, readonly, and in the anonymous category in the .m file override it to nonatomic, copy, readwrite.
Gonna merge for now, and then we can decide on the accessors in initializers style rule later. |
The ultimate goal of this pull request is to make FirebaseUI more familiar for new contributors who are already used to the iOS OSS landscape. Ideally, any developer should be able to fork the repo and make changes with confidence that the unit tests will tell them if their changes have broken anything.
The first step of that is to have tests.