Run unit tests for tvOS #1401
Run unit tests for tvOS #1401
Conversation
| genTestObject(@1, @"Foo"), | ||
| genTestObject(@2, @"Qux"), // new value | ||
| genTestObject(@1, @"Qux"), // new value | ||
| genTestObject(@2, @"Bar"), | ||
| genTestObject(@3, @"Baz"), |
There was a problem hiding this comment.
I changed this so that the first object is updated instead of the second, because on tvOS cellForItemAtIndexPath doesn't return a cell for the second section, presumably because it's out of range.
There was a problem hiding this comment.
because on tvOS cellForItemAtIndexPath doesn't return a cell for the second section, presumably because it's out of range.
Ummm this seems weird, it should return the right section data if the indexPath is correct, can you investigate a bit more why the test doesn't work for tvOS?
There was a problem hiding this comment.
@koenpunt did u figure out why this has to change?
There was a problem hiding this comment.
Not yet, I'll dive into this a bit more tomorrow.
There was a problem hiding this comment.
The behavior of cellForItemAtIndexPath is just different for tvOS. When I change the frame of the collection view to be bigger than 100x100, even only slightly (100x105), the test passes. I have yet to see how this behaves when running in the tvOS simulator, but maybe that will make it clear why this is happening.
There was a problem hiding this comment.
@lorixx has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
| genTestObject(@1, @"Foo"), | ||
| genTestObject(@2, @"Qux"), // new value | ||
| genTestObject(@1, @"Qux"), // new value | ||
| genTestObject(@2, @"Bar"), | ||
| genTestObject(@3, @"Baz"), |
There was a problem hiding this comment.
because on tvOS cellForItemAtIndexPath doesn't return a cell for the second section, presumably because it's out of range.
Ummm this seems weird, it should return the right section data if the indexPath is correct, can you investigate a bit more why the test doesn't work for tvOS?
How do you envision this? That we copy relevant test files to a Another thing I noticed, there are quite a few tests prefixed with |
I think those are flaky tests and we have internal CI to auto-prefix with
Nah, def not duplicate the test files here. If all the tests work for iOS target, we should have pretty good confidence that the code works for tv_os. I would rather we only add tv_os specific test cases instead of de-colocate tests in Also from the optimization perspective, we should def focus more on iOS instead of tv_os, which had way less usage than the iOS use cases. Can you try add @available(iOS 11.0) for those two test cases inline inside |
|
@koenpunt has updated the pull request. Re-import the pull request |
|
| genTestObject(@1, @"Foo"), | ||
| genTestObject(@2, @"Qux"), // new value | ||
| genTestObject(@1, @"Qux"), // new value | ||
| genTestObject(@2, @"Bar"), | ||
| genTestObject(@3, @"Baz"), |
There was a problem hiding this comment.
@koenpunt did u figure out why this has to change?
There was a problem hiding this comment.
@lorixx has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
Lemme just pipe up in here to say I'm looking at this. I'm actually surprised we didn't have the tests running on tvOS before. There's some additional hurdles to handle now (Such as the test XIB files needing to be duplicated and modified for tvOS support), but I'd definitely like to get this set up when we can! |
Changes in this pull request
As mentioned before in #1395, the unit tests are not executed for tvOS on the CI.
As you will see, they won't pass either.
The later commit in this PR removes certain files from the tvOS test target to make them pass. Namely the tests concerning nibs/storyboards, since those are not supported on tvOS.
Checklist
CHANGELOG.mdfor any breaking changes, enhancements, or bug fixes.