Apple TV support 2: Xcode projects and CI (scripts/objc-test.sh) #10227

douglowder · 2016-10-03T21:57:55Z

Motivation *

Second PR for Apple TV support.

Test plan *

Apple TV tests have been added to scripts/objc-test.sh

facebook-github-bot · 2016-10-03T21:58:22Z

By analyzing the blame information on this pull request, we identified @mmmulani and @fkgozali to be potential reviewers.

douglowder · 2016-10-03T23:05:39Z

Hmmm... Travis CI failed because their build doesn't have a tvOS 10 simulator. Will downgrade the build targets and see if that fixes it.

facebook-github-bot · 2016-10-03T23:14:42Z

@dlowder-salesforce updated the pull request - view changes

facebook-github-bot · 2016-10-03T23:58:40Z

@dlowder-salesforce updated the pull request - view changes

douglowder · 2016-10-04T00:33:12Z

@javache here is Apple TV round 2 :)

javache

Looks good!

javache · 2016-10-04T09:19:52Z

React/Views/RCTScrollView.m

@@ -946,6 +955,9 @@ - (type)getter                                            \
 #if !TARGET_OS_TV
 RCT_SET_AND_PRESERVE_OFFSET(setPagingEnabled, isPagingEnabled, BOOL)
 RCT_SET_AND_PRESERVE_OFFSET(setScrollsToTop, scrollsToTop, BOOL)
+#else
+RCT_EMPTY_BOOL_GETTER_SETTER(setPagingEnabled, isPagingEnabled)


Why is this necessary? I believe RN wil automatically ignore attributes that are not exported by a view manager (which is how we OS-specific attributes)

If I didn't make this change, I got an "unrecognized selector" [RCTScrollView setScrollsToTop] when attempting to run a modified version of UIExplorer to try some of the snapshot tests.

Thanks to your hint I found the correct way to do this... committing a fix.

javache · 2016-10-04T09:20:17Z

packager/react-native-xcode.sh

@@ -14,7 +14,7 @@ case "$CONFIGURATION" in
  Debug)
    # Speed up build times by skipping the creation of the offline package for debug
    # builds on the simulator since the packager is supposed to be running anyways.
-    if [[ "$PLATFORM_NAME" = "iphonesimulator" ]]; then
+    if [[ "$PLATFORM_NAME" = "iphonesimulator" || "$PLATFORM_NAME" = "appletvsimulator" ]]; then


Maybe regex match on 'simulator$'?

facebook-github-bot · 2016-10-04T14:58:50Z

@dlowder-salesforce updated the pull request - view changes

facebook-github-bot · 2016-10-04T15:31:54Z

@dlowder-salesforce updated the pull request - view changes

facebook-github-bot · 2016-10-04T15:40:51Z

@dlowder-salesforce updated the pull request - view changes

javache · 2016-10-04T17:14:15Z

Examples/UIExplorer/UIExplorerUnitTests/RCTBundleURLProviderTests.m

@@ -73,6 +73,7 @@ - (void)tearDown

 - (void)testBundleURL
 {
+#if !TARGET_OS_TV


How is this this test broken on tvOS?

Maybe this is an issue with the new Xcode and not with tvOS, but the URL string was using the IP address of the host instead of "localhost". I can revert this change and see if it passes in CI.

javache · 2016-10-04T17:17:23Z

React/Modules/RCTUIManager.h

@@ -97,7 +97,6 @@ RCT_EXTERN NSString *const RCTUIManagerRootViewKey;
 * @param reactTag the component tag
 * @param completion the completion block that will hand over the rootView, if any.
 *
- * @return the rootView


That change fixes an actual doc error -- the method does not in fact return anything.

javache · 2016-10-04T17:18:55Z

React/React.xcodeproj/project.pbxproj

+		2D3B5EEF1D9B09DC00451313 /* RCTViewManager.m in Sources */ = {isa = PBXBuildFile; fileRef = 13E0674E1A70F44B002CDEE1 /* RCTViewManager.m */; };
+		2D3B5EF01D9B09E300451313 /* RCTWrapperViewController.m in Sources */ = {isa = PBXBuildFile; fileRef = 13B080241A694A8400A75B9A /* RCTWrapperViewController.m */; };
+		2D3B5EF11D9B09E700451313 /* UIView+React.m in Sources */ = {isa = PBXBuildFile; fileRef = 13E067541A70F44B002CDEE1 /* UIView+React.m */; };
+		2D8C2E331DA40441000EE098 /* RCTMultipartStreamReader.m in Sources */ = {isa = PBXBuildFile; fileRef = 001BFCCF1D8381DE008E587E /* RCTMultipartStreamReader.m */; };


Any idea why all of these were added?

Those are to include the files in the tvOS build.

javache · 2016-10-04T17:20:20Z

React/Views/RCTModalHostViewController.m

@@ -73,5 +72,7 @@ - (UIInterfaceOrientationMask)supportedInterfaceOrientations
  return _supportedInterfaceOrientations;
 }
 #endif
+#endif


Can you add a comment here to clarify what each of the if's are closing?

javache · 2016-10-04T17:20:35Z

React/Views/RCTScrollView.m

@@ -930,7 +930,7 @@ - (type)getter                                            \
 {                                                         \
  return [_scrollView getter];                            \
 }
-
+  


facebook-github-bot · 2016-10-04T18:06:57Z

@dlowder-salesforce updated the pull request - view changes

facebook-github-bot · 2016-10-05T00:30:30Z

@dlowder-salesforce updated the pull request - view changes

javache · 2016-10-05T10:43:47Z

@facebook-github-bot shipit

facebook-github-bot · 2016-10-05T10:45:21Z

Thanks for importing. If you are a Facebook employee, you can view this diff on Phabricator.

bestander · 2016-10-05T22:56:42Z

@dlowder-salesforce, I think this PR's unit tests are consistently failing:

Executed 123 tests, with 0 failures (0 unexpected) in 5.127 (5.227) seconds
All tests
Test Suite UIExplorer-tvOSIntegrationTests.xctest started
RCTLoggingTests
    ✗ testLogging, ((_lastLogLevel) equal to (RCTLogLevelError)) failed: ("2") is not equal to ("3")
    ✗ testLogging, ((_lastLogMessage) equal to (@"Invoking console.error")) failed: ("Warning: Native component for "RCTDatePicker" does not exist") is not equal to ("Invoking console.error")
    ✗ testLogging,
RCTRootViewIntegrationTests
    ✗ testPropertiesUpdateTest, failed: caught "NSInternalInconsistencyException", "RedBox error: Unhandled JS Exception: undefined is not an object (evaluating 'StatusBarManager.HEIGHT')"
    ✗ testPropertiesUpdateTest,
    ✗ testReactContentSizeUpdateTest, failed: caught "NSInternalInconsistencyException", "RedBox error: Unhandled JS Exception: Module AppRegistry is not a registered callable module (calling runApplication)"

https://travis-ci.org/facebook/react-native/jobs/165251326

ghost · 2016-10-05T23:34:45Z

Weird.... the tests were passing locally.

bestander · 2016-10-05T23:44:35Z

It could be sensitive to CI device performance or XCode version

On 6 October 2016 at 00:36, douglowder notifications@github.com wrote:

Weird.... the tests were passing locally.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#10227 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ACBdWDOtm4WaS-NFkrh0FwxU-Cdv4vQcks5qxDR2gaJpZM4KNFqO
.

ghost · 2016-10-05T23:51:49Z

Hmmm maybe put the Apple TV objc-test.sh in a separate script?

bestander · 2016-10-06T00:18:00Z

Yeah, let's give it a try.
Either this or disable it for now, otherwise we lose signal from all iOS
builds.

On Thursday, 6 October 2016, douglowder notifications@github.com wrote:

Hmmm maybe put the Apple TV objc-test.sh in a separate script?

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#10227 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ACBdWEVCmYzV4TdgZDhZwxVCS8M5w4Xeks5qxDhzgaJpZM4KNFqO
.

Summary: * Motivation * Second PR for Apple TV support. * Test plan * Apple TV tests have been added to scripts/objc-test.sh Closes facebook/react-native#10227 Differential Revision: D3974064 Pulled By: javache fbshipit-source-id: 36dffb4517efa489e40fa713a30655d1d76ef646

Apple TV support 2: Xcode projects and CI (scripts/objc-test.sh)

94fb268

facebook-github-bot added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Oct 3, 2016

Apple TV: use tvOS 9.2 target

8892456

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 3, 2016

Merge remote-tracking branch 'upstream/master'

670bbb9

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 4, 2016

javache reviewed Oct 4, 2016

View reviewed changes

Apple TV: fix exported methods in RCTScrollView

b50b88c

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 4, 2016

Apple TV: use regex for simulator types in react-native-xcode.sh

e599607

douglowder added 2 commits October 4, 2016 08:35

Merge remote-tracking branch 'upstream/master'

5fe1d48

Apple TV: add new MultipartStreamReader code and tests

b726fe8

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 4, 2016

javache reviewed Oct 4, 2016

View reviewed changes

douglowder added 2 commits October 4, 2016 10:55

Merge remote-tracking branch 'upstream/master'

8f8058b

Apple TV: respond to code review for PR facebook#10227

eab7221

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 4, 2016

douglowder added 2 commits October 4, 2016 17:25

Merge remote-tracking branch 'upstream/master'

a2bb962

Apple TV: fix React project after upstream merge

960ca2b

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 5, 2016

facebook-github-bot added GH Review: accepted Import Started This pull request has been imported. This does not imply the PR has been approved. and removed GH Review: review-needed labels Oct 5, 2016

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 5, 2016

facebook-github-bot closed this in 8622998 Oct 5, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Apple TV support 2: Xcode projects and CI (scripts/objc-test.sh) #10227

Apple TV support 2: Xcode projects and CI (scripts/objc-test.sh) #10227

douglowder commented Oct 3, 2016

facebook-github-bot commented Oct 3, 2016

douglowder commented Oct 3, 2016

facebook-github-bot commented Oct 3, 2016

facebook-github-bot commented Oct 3, 2016

douglowder commented Oct 4, 2016

javache left a comment

javache Oct 4, 2016

douglowder Oct 4, 2016

douglowder Oct 4, 2016

javache Oct 4, 2016

douglowder Oct 4, 2016

facebook-github-bot commented Oct 4, 2016

facebook-github-bot commented Oct 4, 2016

facebook-github-bot commented Oct 4, 2016

javache Oct 4, 2016

douglowder Oct 4, 2016

douglowder Oct 4, 2016

javache Oct 4, 2016

douglowder Oct 4, 2016

javache Oct 4, 2016

douglowder Oct 4, 2016

javache Oct 4, 2016

douglowder Oct 4, 2016

javache Oct 4, 2016

douglowder Oct 4, 2016

facebook-github-bot commented Oct 4, 2016

facebook-github-bot commented Oct 5, 2016

javache commented Oct 5, 2016

facebook-github-bot commented Oct 5, 2016

bestander commented Oct 5, 2016

ghost commented Oct 5, 2016

bestander commented Oct 5, 2016

ghost commented Oct 5, 2016

bestander commented Oct 6, 2016

Apple TV support 2: Xcode projects and CI (scripts/objc-test.sh) #10227

Apple TV support 2: Xcode projects and CI (scripts/objc-test.sh) #10227

Conversation

douglowder commented Oct 3, 2016

facebook-github-bot commented Oct 3, 2016

douglowder commented Oct 3, 2016

facebook-github-bot commented Oct 3, 2016

facebook-github-bot commented Oct 3, 2016

douglowder commented Oct 4, 2016

javache left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Oct 4, 2016

facebook-github-bot commented Oct 4, 2016

facebook-github-bot commented Oct 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Oct 4, 2016

facebook-github-bot commented Oct 5, 2016

javache commented Oct 5, 2016

facebook-github-bot commented Oct 5, 2016

bestander commented Oct 5, 2016

ghost commented Oct 5, 2016

bestander commented Oct 5, 2016

ghost commented Oct 5, 2016

bestander commented Oct 6, 2016