-
Notifications
You must be signed in to change notification settings - Fork 24.9k
fix(test): fix RNTester iOS unit and integration tests #53848
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
base: main
Are you sure you want to change the base?
Conversation
…ifecycle-originating updates from DeviceEventEmitter
…s, do not attach debugger to tests by default
9e9630c
to
c9484f1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolutely, thanks for the comments, I'll address them today. |
… the default export in IntegrationTestsApp
…nUtils.set_react_codegen_podspec_generated
original_define_singleton_method = CodegenUtils.method(:assert_codegen_folder_is_empty) | ||
CodegenUtils.define_singleton_method(:assert_codegen_folder_is_empty) do |*args, **kwargs| | ||
# no-op | ||
end | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This mock was missing
"/MyModuleSpecs/MyModule.h", | ||
"#{codegen_path}/MyModuleSpecs/MyModule.mm", | ||
"#{codegen_path}/react/components/MyComponent/ShadowNode.h", | ||
"#{codegen_path}/react/components/MyComponent/ShadowNode.mm", | ||
codegen_path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this baseline, these files apparently are no longer present anywhere
# Assert | ||
assert_equal($podInvocationCount, 3) | ||
assert_equal($podInvocation["React-jsi"][:path], "../../ReactCommon/jsi") | ||
assert_equal($podInvocationCount, 2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The setup_hermes
helper now only adds 2 pods
folly_config = Helpers::Constants.folly_config | ||
folly_compiler_flags = folly_config[:compiler_flags] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is unused. The NewArchitectureHelper
has a getter of that name, but no setter. I tried implementing it, but test results haven't change, so I assumed this is dead code. WDYT @cipolleschi ?
assert_equal(spec.pod_target_xcconfig["HEADER_SEARCH_PATHS"], "\"$(PODS_ROOT)/boost\" \"$(PODS_ROOT)/Headers/Private/Yoga\" \"$(PODS_ROOT)/DoubleConversion\" \"$(PODS_ROOT)/fast_float/include\" \"$(PODS_ROOT)/fmt/include\" \"${PODS_CONFIGURATION_BUILD_DIR}/React-graphics/React_graphics.framework/Headers\" \"${PODS_CONFIGURATION_BUILD_DIR}/React-graphics/React_graphics.framework/Headers/react/renderer/graphics/platform/ios\" \"${PODS_CONFIGURATION_BUILD_DIR}/React-Fabric/React_Fabric.framework/Headers\" \"${PODS_CONFIGURATION_BUILD_DIR}/React-Fabric/React_Fabric.framework/Headers/react/renderer/components/view/platform/cxx\" \"${PODS_CONFIGURATION_BUILD_DIR}/React-FabricImage/React_FabricImage.framework/Headers\" \"${PODS_CONFIGURATION_BUILD_DIR}/ReactCommon/ReactCommon.framework/Headers\" \"${PODS_CONFIGURATION_BUILD_DIR}/ReactCommon/ReactCommon.framework/Headers/react/nativemodule/core\" \"${PODS_CONFIGURATION_BUILD_DIR}/React-NativeModulesApple/React_NativeModulesApple.framework/Headers\" \"${PODS_CONFIGURATION_BUILD_DIR}/React-RCTFabric/RCTFabric.framework/Headers\" \"${PODS_CONFIGURATION_BUILD_DIR}/React-utils/React_utils.framework/Headers\" \"${PODS_CONFIGURATION_BUILD_DIR}/React-featureflags/React_featureflags.framework/Headers\" \"${PODS_CONFIGURATION_BUILD_DIR}/React-debug/React_debug.framework/Headers\" \"${PODS_CONFIGURATION_BUILD_DIR}/React-ImageManager/React_ImageManager.framework/Headers\" \"${PODS_CONFIGURATION_BUILD_DIR}/React-rendererdebug/React_rendererdebug.framework/Headers\"") | ||
assert_equal( | ||
spec.pod_target_xcconfig["HEADER_SEARCH_PATHS"], | ||
[ | ||
"\"$(PODS_ROOT)/Headers/Private/Yoga\"", | ||
"$(PODS_ROOT)/glog", | ||
"$(PODS_ROOT)/boost", | ||
"$(PODS_ROOT)/DoubleConversion", | ||
"$(PODS_ROOT)/fast_float/include", | ||
"$(PODS_ROOT)/fmt/include", | ||
"$(PODS_ROOT)/SocketRocket", | ||
"$(PODS_ROOT)/RCT-Folly" | ||
] | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This apparently is the correct baseline currently
) | ||
assert_equal(spec.pod_target_xcconfig["CLANG_CXX_LANGUAGE_STANDARD"], "c++20") | ||
assert_equal(spec.pod_target_xcconfig["OTHER_CPLUSPLUSFLAGS"], "$(inherited) -DRCT_NEW_ARCH_ENABLED=1") | ||
assert_equal(spec.pod_target_xcconfig["OTHER_CPLUSPLUSFLAGS"], "$(inherited) -DRCT_NEW_ARCH_ENABLED=1 ") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will touch not only this line, but all other occurences of -DRCT_NEW_ARCH_ENABLED=1
in the baselines: apparently, the current implementation appends a trailing whitespace to the define (source here). Even in the same file, it appears that the whitespace is intentionally being stripped in one place. I tried stripping it and the tests still pass. I wanted to consult with you Riccardo @cipolleschi if you agree that we are safe to strip this one?
assert_equal(first_xcconfig.attributes["OTHER_CPLUSPLUSFLAGS"], "$(inherited) -DFOLLY_NO_CONFIG -DFOLLY_MOBILE=1 -DFOLLY_USE_LIBCPP=1 -DFOLLY_CFG_NO_COROUTINES=1 -DFOLLY_HAVE_CLOCK_GETTIME=1 -Wno-comma -Wno-shorten-64-to-32") | ||
assert_equal(first_xcconfig.attributes["OTHER_CPLUSPLUSFLAGS"], "$(inherited) ") | ||
assert_equal(first_xcconfig.save_as_invocation, ["a/path/First.xcconfig"]) | ||
assert_equal(second_xcconfig.attributes["OTHER_CPLUSPLUSFLAGS"], "$(inherited) -DFOLLY_NO_CONFIG -DFOLLY_MOBILE=1 -DFOLLY_USE_LIBCPP=1 -DFOLLY_CFG_NO_COROUTINES=1 -DFOLLY_HAVE_CLOCK_GETTIME=1 -Wno-comma -Wno-shorten-64-to-32") | ||
assert_equal(second_xcconfig.attributes["OTHER_CPLUSPLUSFLAGS"], "$(inherited) ") | ||
assert_equal(second_xcconfig.save_as_invocation, ["a/path/Second.xcconfig"]) | ||
assert_equal(react_core_debug_config.build_settings["OTHER_CPLUSPLUSFLAGS"], "$(inherited) -DFOLLY_NO_CONFIG -DFOLLY_MOBILE=1 -DFOLLY_USE_LIBCPP=1 -DFOLLY_CFG_NO_COROUTINES=1 -DFOLLY_HAVE_CLOCK_GETTIME=1 -Wno-comma -Wno-shorten-64-to-32") | ||
assert_equal(react_core_release_config.build_settings["OTHER_CPLUSPLUSFLAGS"], "$(inherited) -DFOLLY_NO_CONFIG -DFOLLY_MOBILE=1 -DFOLLY_USE_LIBCPP=1 -DFOLLY_CFG_NO_COROUTINES=1 -DFOLLY_HAVE_CLOCK_GETTIME=1 -Wno-comma -Wno-shorten-64-to-32") | ||
assert_equal(react_core_debug_config.build_settings["OTHER_CPLUSPLUSFLAGS"], "$(inherited) ") | ||
assert_equal(react_core_release_config.build_settings["OTHER_CPLUSPLUSFLAGS"], "$(inherited) ") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cipolleschi Riccardo, could you verify this is correct behaviour for an old arch scenario?
expected_search_path = "$(inherited) ${PODS_CONFIGURATION_BUILD_DIR}/ReactCommon/ReactCommon.framework/Headers ${PODS_CONFIGURATION_BUILD_DIR}/ReactCommon/ReactCommon.framework/Headers/react/nativemodule/core ${PODS_CONFIGURATION_BUILD_DIR}/React-runtimeexecutor/React_runtimeexecutor.framework/Headers ${PODS_CONFIGURATION_BUILD_DIR}/React-runtimeexecutor/React_runtimeexecutor.framework/Headers/platform/ios ${PODS_CONFIGURATION_BUILD_DIR}/ReactCommon-Samples/ReactCommon_Samples.framework/Headers ${PODS_CONFIGURATION_BUILD_DIR}/ReactCommon-Samples/ReactCommon_Samples.framework/Headers/platform/ios ${PODS_CONFIGURATION_BUILD_DIR}/React-Fabric/React_Fabric.framework/Headers/react/renderer/components/view/platform/cxx ${PODS_CONFIGURATION_BUILD_DIR}/React-NativeModulesApple/React_NativeModulesApple.framework/Headers ${PODS_CONFIGURATION_BUILD_DIR}/React-graphics/React_graphics.framework/Headers ${PODS_CONFIGURATION_BUILD_DIR}/React-graphics/React_graphics.framework/Headers/react/renderer/graphics/platform/ios" | ||
assert_equal(expected_search_path, received_search_path) | ||
end | ||
|
||
installer.target_installation_results.pod_target_installation_results.each do |pod_name, target_installation_result| | ||
if pod_name == "SecondTarget" | ||
target_installation_result.native_target.build_configurations.each do |config| | ||
received_search_path = config.build_settings["HEADER_SEARCH_PATHS"] | ||
expected_Search_path = "$(inherited) \"$(PODS_ROOT)/RCT-Folly\" \"$(PODS_ROOT)/DoubleConversion\" \"$(PODS_ROOT)/fast_float/include\" \"$(PODS_ROOT)/fmt/include\" \"$(PODS_ROOT)/boost\" \"${PODS_CONFIGURATION_BUILD_DIR}/ReactCodegen/ReactCodegen.framework/Headers\" \"${PODS_CONFIGURATION_BUILD_DIR}/ReactCommon/ReactCommon.framework/Headers\" \"${PODS_CONFIGURATION_BUILD_DIR}/ReactCommon/ReactCommon.framework/Headers/react/nativemodule/core\" \"${PODS_CONFIGURATION_BUILD_DIR}/React-RCTFabric/RCTFabric.framework/Headers\" \"${PODS_CONFIGURATION_BUILD_DIR}/React-Fabric/React_Fabric.framework/Headers\" \"${PODS_CONFIGURATION_BUILD_DIR}/React-Fabric/React_Fabric.framework/Headers/react/renderer/components/view/platform/cxx\" \"${PODS_CONFIGURATION_BUILD_DIR}/React-FabricImage/React_FabricImage.framework/Headers\" \"${PODS_CONFIGURATION_BUILD_DIR}/React-Graphics/React_graphics.framework/Headers\" \"${PODS_CONFIGURATION_BUILD_DIR}/React-Graphics/React_graphics.framework/Headers/react/renderer/graphics/platform/ios\" \"${PODS_CONFIGURATION_BUILD_DIR}/React-Fabric/React_Fabric.framework/Headers/react/renderer/imagemanager/platform/ios\"" | ||
expected_Search_path = "$(inherited) \"$(PODS_ROOT)/RCT-Folly\" \"$(PODS_ROOT)/DoubleConversion\" \"$(PODS_ROOT)/fast_float/include\" \"$(PODS_ROOT)/fmt/include\" \"$(PODS_ROOT)/boost\" \"${PODS_CONFIGURATION_BUILD_DIR}/ReactCodegen/ReactCodegen.framework/Headers\" \"${PODS_CONFIGURATION_BUILD_DIR}/ReactCommon/ReactCommon.framework/Headers\" \"${PODS_CONFIGURATION_BUILD_DIR}/ReactCommon/ReactCommon.framework/Headers/react/nativemodule/core\" \"${PODS_CONFIGURATION_BUILD_DIR}/React-runtimeexecutor/React_runtimeexecutor.framework/Headers\" \"${PODS_CONFIGURATION_BUILD_DIR}/React-runtimeexecutor/React_runtimeexecutor.framework/Headers/platform/ios\" \"${PODS_CONFIGURATION_BUILD_DIR}/React-RCTFabric/RCTFabric.framework/Headers\" \"${PODS_CONFIGURATION_BUILD_DIR}/React-Fabric/React_Fabric.framework/Headers\" \"${PODS_CONFIGURATION_BUILD_DIR}/React-Fabric/React_Fabric.framework/Headers/react/renderer/components/view/platform/cxx\" \"${PODS_CONFIGURATION_BUILD_DIR}/React-FabricImage/React_FabricImage.framework/Headers\" \"${PODS_CONFIGURATION_BUILD_DIR}/React-Graphics/React_graphics.framework/Headers\" \"${PODS_CONFIGURATION_BUILD_DIR}/React-Graphics/React_graphics.framework/Headers/react/renderer/graphics/platform/ios\" \"${PODS_CONFIGURATION_BUILD_DIR}/React-Fabric/React_Fabric.framework/Headers/react/renderer/imagemanager/platform/ios\"" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These have apparently changed, too.
installer.target_installation_results.pod_target_installation_results.each do |pod_name, target_installation_result| | ||
if pod_name.to_s == target_pod_name | ||
target_installation_result.native_target.build_configurations.each do |config| | ||
if configuration_type == nil || (configuration_type != nil && config.type == configuration_type) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this file, indentation was messed up
# Assert | ||
assert_equal(DirMock.exist_invocation_params, [codegen_path, codegen_path]) | ||
assert_equal(DirMock.glob_invocation, ["#{codegen_path}/*", "#{codegen_path}/*"]) | ||
assert_equal(DirMock.exist_invocation_params, [codegen_path]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is related to most (although, not all) assert_equal
invocations in Ruby tests: the order of arguments is inverted, i.e., expected is swapped with actual. The signature is assert_equal(exp, act, msg = nil)
, while here and in all other places, the first argument is the actual value and the second is the expected value.
This deteriorates DX since makes it misleading to browse the messages of failed assertions:
e54a3d9
to
b37ee40
Compare
b37ee40
to
fe3d3de
Compare
Summary:
This PR fixes problems with unit & integration tests in the iOS RNTester project, related to: compilation errors, JS problems, missing mocks & outdated baselines.
Changelog:
[GENERAL] [FIXED] - IntegrationTestsApp not registering components under proper names, breaking integration tests
[GENERAL] [FIXED] - IntegrationTestsApp flow types
[GENERAL] [FIXED] - Re-enabled iOS unit & integration tests, Ruby tests on CI
[IOS] [FIXED] - Fixed Ruby scripting tests, refactored order of arguments to
assert_equals
where it was swapped[IOS] [FIXED] - Reconfigured CI iOS test runner to run on iPhone 16, iOS 18.1, since the previous device was missing in the runner setup
[IOS] [FIXED] - Compilation errors of iOS unit tests
[IOS] [FIXED] - Fixed logic of broken iOS tests, add missing mocks
Test Plan: