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.

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

Run unit tests for tvOS #1401

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Run unit tests for tvOS #1401

wants to merge 2 commits into from

Conversation

koenpunt
Copy link
Contributor

@koenpunt koenpunt commented Nov 24, 2019

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

  • All tests pass. Demo project builds and runs.
  • I added tests, an experiment, or detailed why my change isn't tested.
  • I added an entry to the CHANGELOG.md for any breaking changes, enhancements, or bug fixes.
  • I have reviewed the contributing guide

Comment on lines -78 to 80
genTestObject(@1, @"Foo"),
genTestObject(@2, @"Qux"), // new value
genTestObject(@1, @"Qux"), // new value
genTestObject(@2, @"Bar"),
genTestObject(@3, @"Baz"),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

@koenpunt did u figure out why this has to change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not yet, I'll dive into this a bit more tomorrow.

Copy link
Contributor Author

@koenpunt koenpunt Nov 27, 2019

Choose a reason for hiding this comment

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

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.

@koenpunt koenpunt marked this pull request as ready for review November 25, 2019 10:47
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@lorixx has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Copy link
Contributor

@lorixx lorixx left a comment

Choose a reason for hiding this comment

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

Thanks for looking into the tvOS tests support! Overall the biggest feedback I have is not to have this special treatment to remove test code colocation for IGListAdapterTests. We'd better create a different set of Test files for tvOS.

Tests/IGListAdapterTests.m Show resolved Hide resolved
Comment on lines -78 to 80
genTestObject(@1, @"Foo"),
genTestObject(@2, @"Qux"), // new value
genTestObject(@1, @"Qux"), // new value
genTestObject(@2, @"Bar"),
genTestObject(@3, @"Baz"),
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

@koenpunt
Copy link
Contributor Author

We'd better create a different set of Test files for tvOS.

How do you envision this? That we copy relevant test files to a TVOSTests folder or something? But then again, almost all tests pass for tvOS, so why would you want to duplicate tests?

Another thing I noticed, there are quite a few tests prefixed with DISABLED_, do we have to keep them?

@lorixx
Copy link
Contributor

lorixx commented Nov 26, 2019

DISABLED_, do we have to keep them?

I think those are flaky tests and we have internal CI to auto-prefix with DISABLED_ to disable the test, the solution should actually fix the broken fix and remove that prefix here.

That we copy relevant test files to a TVOSTests folder or something?

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 IGListAdapterTests.m.

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 IGListAdapterTests.m ?

@facebook-github-bot
Copy link
Contributor

@koenpunt has updated the pull request. Re-import the pull request

@koenpunt
Copy link
Contributor Author

Can you try add @available(iOS 11.0) for those two test cases inline inside IGListAdapterTests.m ?

@available didn't work, but using the TARGET_OS_TV macro does.

Comment on lines -78 to 80
genTestObject(@1, @"Foo"),
genTestObject(@2, @"Qux"), // new value
genTestObject(@1, @"Qux"), // new value
genTestObject(@2, @"Bar"),
genTestObject(@3, @"Baz"),
Copy link
Contributor

Choose a reason for hiding this comment

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

@koenpunt did u figure out why this has to change?

Tests/IGListAdapterTests.m Show resolved Hide resolved
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@lorixx has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@TimOliver
Copy link
Member

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants