Fixing CircleCI and Travis builds. #1473
Conversation
|
I know this will fail. We need to do a release on Bolts to really get this working but I wanted this hear so that I can test our build and make adjustments as we update bolts. |
This reverts commit 912dc29.
noobs2ninjas
changed the title
|
God I hate the fact that Xcode environment renames all simulators every 2 updates. |
|
Finally! |
|
@drdaz did you want to review changes or should I just push it on through? I think I finally got everything figured out and got it done. Updated to Xcode 13.2.1. Working off the Bolts-ObjC fork. Updated submodules(xctoolchain was a pain). Fixed Travis issues. CircleCI builds and the only thing failing is unit tests. Also, got the catalyst mess figured out that screwed everything up when we updated with all the framework name ambiguity and the poor separation between platform targets because of it. Only took me 2000 commits to figure everything out! Good news is that I feel I kinda got the hang of this now. Now I think we do need to do some Catalyst specific testing to improve compatibility but right now I just want to get a release out and get everything back on track. Edit: Nevermind. Have to wait until xctoolchain is updated again because Travis doesnt update their documentation. |
|
I'm confused why you merged this to master? None of the tests aside from CI seem to have been working here?? |
|
Because all the tests BUILD. The only reason things are failing now is due
to failed unit tests. Which means my this pull request did it’s job.
…On Sat, Jan 4, 2020 at 4:38 AM Darren Black ***@***.***> wrote:
I'm a confused why you merged this to master? None of the tests aside from
CI seem to have been working here??
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
<#1473?email_source=notifications&email_token=AA2BOXZQYF7ILIPRRB7MV6LQ4BRKRA5CNFSM4JVD6WHKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEICVI4Y#issuecomment-570774643>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA2BOX767G5OFJNMQKEQHNDQ4BRKRANCNFSM4JVD6WHA>
.
|
|
Oh I get what you are pointing at. No I canceled those builds because it
takes forever for them to run and it takes away from other repos trying to
run since we can only have 1 building concurrently. I merged without
approval so you could catch up to master. So yea. Shows failed because I
canceled.
On Sun, Jan 5, 2020 at 7:59 AM Nathan Kellert <[email protected]>
wrote:
… Because all the tests BUILD. The only reason things are failing now is due
to failed unit tests. Which means my this pull request did it’s job.
On Sat, Jan 4, 2020 at 4:38 AM Darren Black ***@***.***>
wrote:
> I'm a confused why you merged this to master? None of the tests aside
> from CI seem to have been working here??
>
> —
> You are receiving this because you modified the open/close state.
> Reply to this email directly, view it on GitHub
> <#1473?email_source=notifications&email_token=AA2BOXZQYF7ILIPRRB7MV6LQ4BRKRA5CNFSM4JVD6WHKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEICVI4Y#issuecomment-570774643>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AA2BOX767G5OFJNMQKEQHNDQ4BRKRANCNFSM4JVD6WHA>
> .
>
|
Well, no. The changes here cause the tests to fail to build. Pretty much all the tests in master fail on the OCMock dependency now, and that's somehow the result of the changes in this PR: https://circleci.com/gh/parse-community/Parse-SDK-iOS-OSX/4245
You only merge to master when you can be reasonably sure your changes don't break things. That means actually executing the tests we have. Running the tests locally is much faster, frees up our build server, and reduces commit spam. EDIT: But letting the build server run the tests is still vital, since it's a clean system every time. Our local machines can have things that change build and test results.
There's no reason to update to a codebase where the tests are newly broken (the current state of master), because that codebase needs fixing. If I update to master, I won't be able to test my changes anymore without figuring out where things went wrong. Similarly, people forking master at this point to, say, fix an issue, will be starting with a broken codebase and have have no idea what effects their changes have on existing functionality. You asked for my response on this PR, but didn't wait for me to actually respond. There really is a reason for the review system, and we really need to follow it. |
|
I think we are not understanding each other. The last things I did with the CircleCI build had every single test building(with the exception of nightly builds). I did continue to make changes but nothing I thought had anything to do with CircleCI. I guess I should have gone back and let it all run after I fixed travis. You are right and thats my bad. I'll take a look and get it fixed. Dang, I was so positive everything was still building I didnt even think about it. |
|
Also, I'll do better in the future with approval. Thats my bad. |
|
@drdaz |
|
I'm wondering whether it might be best to add branch protection on master to require at least one review, just so there is no confusion in future, I'm not pointing fingers - I've merged without review in the past but I've come to see how important the review process is. Thoughts? |
|
@TomWFox I'm 100% behind that idea. It's a useful rule, and we may as well enforce it. |
|
FWIW I'd recommend reverting the merge to master and taking a proper look at this. We can still safely do that since there haven't been any commits to master since. |
Uh oh!
There was an error while loading. Please reload this page.