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.

Sign up for GitHub

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

Jump to bottom

Improve ObjC bazel test and add TODOs. #29665

Merged
merged 7 commits into from
May 18, 2022
Merged

Improve ObjC bazel test and add TODOs. #29665

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
68cfae7
objC bazel test: run InteropTests and MacTests
jtattermusch May 12, 2022
46a8199
objC bazel test: run TvTests
jtattermusch May 12, 2022
c65b474
Revert "objC bazel test: run TvTests"
jtattermusch May 12, 2022
b10b579
avoid running flaky objC InteropTests under bazel
jtattermusch May 12, 2022
d231f31
add TODOs for objc run_tests.py tasks
jtattermusch May 12, 2022
fc4172d
add a note about GRPC_VERBOSITY=debug being set
jtattermusch May 12, 2022
7f93cf4
increase timeout for ios-buildtest-example-tvOS-sample test task
jtattermusch May 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 7 additions & 4 deletions tools/internal_ci/macos/grpc_objc_bazel_test.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,9 @@ EXAMPLE_TARGETS=(

TEST_TARGETS=(
# TODO(jtattermusch): ideally we'd say "//src/objective-c/tests/..." but not all the targets currently build
# TODO(jtattermusch): make //src/objective-c/tests:TvTests build reliably
# TODO(jtattermusch): make //src/objective-c/tests:MacTests build reliably
# TODO(jtattermusch): make //src/objective-c/tests:TvTests test pass with bazel
# TODO(jtattermusch): make sure the //src/objective-c/tests:InteropTests test passes reliably under bazel
//src/objective-c/tests:MacTests
//src/objective-c/tests:UnitTests
)

Expand All @@ -74,10 +75,12 @@ INTEROP_SERVER_BINARY=bazel-bin/test/cpp/interop/interop_server
trap 'echo "KILLING interop_server binaries running on the background"; kill -9 $(jobs -p)' EXIT
# === END SECTION: run interop_server on the background ====

# TODO(jtattermusch): set GRPC_VERBOSITY=debug when running tests on a simulator (how to do that?)

python3 tools/run_tests/python_utils/bazel_report_helper.py --report_path objc_bazel_tests

# NOTE: When using bazel to run the tests, test env variables like GRPC_VERBOSITY or GRPC_TRACE
# seem to be correctly applied to the test environment even when running tests on a simulator.
# The below configuration runs all the tests with --test_env=GRPC_VERBOSITY=debug, which makes
# the test logs much more useful.
objc_bazel_tests/bazel_wrapper \
--bazelrc=tools/remote_build/include/test_locally_with_resultstore_results.bazelrc \
test \
Expand Down
27 changes: 25 additions & 2 deletions tools/run_tests/run_tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -981,6 +981,7 @@ def configure(self, config, args):

def test_specs(self):
out = []
# TODO(jtattermusch): Remove this task since the sample is already being built as part of the grpc_objc_bazel_test job.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dennycd let's make sure we have a plan (issue filed) for addressing most TODOs being added in this file.
For some we already have an issue filed, but I think we've missed many of them (some of them quite important).

out.append(
self.config.job_spec(
['src/objective-c/tests/build_one_example_bazel.sh'],
Expand All @@ -993,6 +994,7 @@ def test_specs(self):
'FRAMEWORKS': 'NO'
}))
# Currently not supporting compiling as frameworks in Bazel
# TODO(jtattermusch): verify the above claim is still accurate.
out.append(
self.config.job_spec(
['src/objective-c/tests/build_one_example.sh'],
Expand All @@ -1004,6 +1006,7 @@ def test_specs(self):
'EXAMPLE_PATH': 'src/objective-c/examples/Sample',
'FRAMEWORKS': 'YES'
}))
# TODO(jtattermusch): Create bazel target for the sample and remove the test task from here.
out.append(
self.config.job_spec(
['src/objective-c/tests/build_one_example.sh'],
Expand All @@ -1014,10 +1017,11 @@ def test_specs(self):
'SCHEME': 'SwiftSample',
'EXAMPLE_PATH': 'src/objective-c/examples/SwiftSample'
}))
# TODO(jtattermusch): Remove this task since the sample is already being built as part of the grpc_objc_bazel_test job.
out.append(
self.config.job_spec(
['src/objective-c/tests/build_one_example_bazel.sh'],
timeout_seconds=10 * 60,
timeout_seconds=20 * 60,
shortname='ios-buildtest-example-tvOS-sample',
cpu_cost=1e6,
environ={
Expand All @@ -1038,70 +1042,88 @@ def test_specs(self):
# 'EXAMPLE_PATH': 'src/objective-c/examples/watchOS-sample',
# 'FRAMEWORKS': 'NO'
# }))

# TODO(jtattermusch): Create bazel target for the test and remove the test from here
out.append(
self.config.job_spec(['src/objective-c/tests/run_plugin_tests.sh'],
timeout_seconds=60 * 60,
shortname='ios-test-plugintest',
cpu_cost=1e6,
environ=_FORCE_ENVIRON_FOR_WRAPPERS))
# Note that this test basically tests whether the codegen plugin works correctly by running protoc and checking the contents of the generated *.pbrpc.* files.
# it doesn't really build any ObjC code.
# TODO(jtattermusch): turn this test into a bazel test or come up with a better place where to put this test.
out.append(
self.config.job_spec(
['src/objective-c/tests/run_plugin_option_tests.sh'],
timeout_seconds=60 * 60,
shortname='ios-test-plugin-option-test',
cpu_cost=1e6,
environ=_FORCE_ENVIRON_FOR_WRAPPERS))
# TODO(jtattermusch): move the test out of the test/core/iomgr/CFStreamTests directory?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

do we know why the test lives under test/core/iomgr/ios/CFStreamTests?

Btw, this test runs under various sanitizers (asan, tsan, msan).
https://github.com/grpc/grpc/blob/03e9ac6f1f3132b81a2133ed63864fc72cd93e57/test/core/iomgr/ios/CFStreamTests/build_and_run_tests.sh

Copy link
Contributor

Choose a reason for hiding this comment

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

these are test suite for iOS's CFStream iomgr fork which are implemented as part of the c-core. Ideally we should move these out of test/core and have them co-located with other iOS test suites.

# How does one add the cfstream dependency in bazel?
out.append(
self.config.job_spec(
['test/core/iomgr/ios/CFStreamTests/build_and_run_tests.sh'],
timeout_seconds=60 * 60,
shortname='ios-test-cfstream-tests',
cpu_cost=1e6,
environ=_FORCE_ENVIRON_FOR_WRAPPERS))
# TODO(jtattermusch): Create bazel target for the test and remove the test from here
# TODO(jtattermusch): Clarify what do these tests do?
out.append(
self.config.job_spec(
['src/objective-c/tests/CoreTests/build_and_run_tests.sh'],
timeout_seconds=60 * 60,
shortname='ios-test-core-tests',
cpu_cost=1e6,
environ=_FORCE_ENVIRON_FOR_WRAPPERS))
# TODO: replace with run_one_test_bazel.sh when Bazel-Xcode is stable
# TODO(jtattermusch): Remove this task since the tests are already being run as part of the grpc_objc_bazel_test job.
out.append(
self.config.job_spec(['src/objective-c/tests/run_one_test.sh'],
timeout_seconds=60 * 60,
shortname='ios-test-unittests',
cpu_cost=1e6,
environ={'SCHEME': 'UnitTests'}))
# TODO(jtattermusch): Make sure the //src/objective-c/tests:InteropTests bazel test passes reliably and remove the test from there.
out.append(
self.config.job_spec(['src/objective-c/tests/run_one_test.sh'],
timeout_seconds=60 * 60,
shortname='ios-test-interoptests',
cpu_cost=1e6,
environ={'SCHEME': 'InteropTests'}))
# TODO(jtattermusch): Create bazel target for the test and remove the test from here
# (how does one add the cronet dependency in bazel?)
out.append(
self.config.job_spec(['src/objective-c/tests/run_one_test.sh'],
timeout_seconds=60 * 60,
shortname='ios-test-cronettests',
cpu_cost=1e6,
environ={'SCHEME': 'CronetTests'}))
# TODO(jtattermusch): Create bazel target for the test and remove the test from here.
out.append(
self.config.job_spec(['src/objective-c/tests/run_one_test.sh'],
timeout_seconds=30 * 60,
shortname='ios-perf-test',
cpu_cost=1e6,
environ={'SCHEME': 'PerfTests'}))
# TODO(jtattermusch): Clarify what's the difference between PerfTests and PerfTestsPosix
# TODO(jtattermusch): Create bazel target for the test and remove the test from here.
out.append(
self.config.job_spec(['src/objective-c/tests/run_one_test.sh'],
timeout_seconds=30 * 60,
shortname='ios-perf-test-posix',
cpu_cost=1e6,
environ={'SCHEME': 'PerfTestsPosix'}))
# TODO(jtattermusch): Create bazel target for the test (how does one add the cronet dependency in bazel?)
# TODO(jtattermusch): move the test out of the test/cpp/ios directory?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we know why the test lives under test/cpp/ios?

https://github.com/grpc/grpc/blob/03e9ac6f1f3132b81a2133ed63864fc72cd93e57/test/cpp/ios/build_and_run_tests.sh

Copy link
Contributor

Choose a reason for hiding this comment

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

similar to CFStream test suite, these are for Cronet transport test run under iOS test environment. Will add a task to follow up and move these out of test/cpp/ios

out.append(
self.config.job_spec(['test/cpp/ios/build_and_run_tests.sh'],
timeout_seconds=60 * 60,
shortname='ios-cpp-test-cronet',
cpu_cost=1e6,
environ=_FORCE_ENVIRON_FOR_WRAPPERS))
# TODO(jtattermusch): Remove this task since the tests are already being run as part of the grpc_objc_bazel_test job.
out.append(
self.config.job_spec(['src/objective-c/tests/run_one_test.sh'],
timeout_seconds=60 * 60,
Expand All @@ -1111,6 +1133,7 @@ def test_specs(self):
'SCHEME': 'MacTests',
'PLATFORM': 'macos'
}))
# TODO(jtattermusch): Make sure the //src/objective-c/tests:TvTests bazel test passes and remove the test from here.
out.append(
self.config.job_spec(['src/objective-c/tests/run_one_test.sh'],
timeout_seconds=30 * 60,
Expand Down