search

pivotal-legacy / PivotalCoreKit

Shared library and test code for iOS and macOS projects
http://pivotallabs.com
Other
168 stars 85 forks source link

Nullability annotations #194

Closed xtreme-andy-shen closed 7 years ago

xtreme-andy-shen commented 7 years ago

Rebase and fix of https://github.com/pivotal/PivotalCoreKit/pull/168

Looking to move PCK to better support Swift going forward.

akitchen commented 7 years ago

Awesome, thank you @xtreme-andy-shen !

Will review and merge shortly. Release to follow...

xtreme-andy-shen commented 7 years ago

@akitchen thanks! I'm also adding some more functionality coming from other project. Will have PRs for them as well. I'm wondering why Travis fails, I'm running tests locally and it's passing.

akitchen commented 7 years ago

We have a large Swift project trying this out today. Separately, I'll have a look at Travis –– I had some trouble with it too around the last release.

On Mar 1, 2017, at 07:02, Andy Shen notifications@github.com wrote:

@akitchen thanks! I'm also adding some more functionality coming from other project. Will have PRs for them as well. I'm wondering why Travis fails, I'm running tests locally and it's passing.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

xtreme-andy-shen commented 7 years ago

@akitchen thanks! Keep me updated, I'm interested, our project is also looking to integrate.

akitchen commented 7 years ago

I ran rake travis locally and it passed. There are only a few places in the code where behavior has been altered, and only subtly ... so I've re-started the travis build and will try to see if we can get to the bottom of it.

akitchen commented 7 years ago

So, it appears that there is a race condition behind that failing test, which has only started surfacing as Travis seems to be getting slower and slower for us.

Sorry that your PR drew the short straw here, but we need to fix this before we can push more code to a red CI. I'm looking into the best way to fix it, and am open to any opinions people may have on the subject.

Cc: @tjarratt @younata

On Mar 1, 2017, at 11:46, Andy Shen notifications@github.com wrote:

@akitchen thanks! Keep me updated, I'm interested, our project is also looking to integrate.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

akitchen commented 7 years ago

Thinking about it a bit further, I'm not sure the assertion that's failing is valid to begin with. The connection is being forced to time out while actively loading data.

I'd be inclined to remove the assertions related to the returned data in the test and move on. Given this is NSURLConnection stuff we should probably be deprecating it anyways.

Thoughts?

xtreme-andy-shen commented 7 years ago

@akitchen I'm inclined to remove that as well. Don't see the integrity of the test to be impacted from this removal.

akitchen commented 7 years ago

I've added a backlog item to schedule deprecation of the class involved. I think it's a better use of time vs. fixing the race condition or reasoning about the correct expected behavior in the situation under test there.

thanks again for the PR. I'll work through the rest and cut a new release as soon as I can.