[Objc] add ios libuv timer test #28570

HannahShiSFB · 2022-01-14T20:46:10Z

libuv test source is build as libuv_test library and selected test cases will be ran from objective C code.

Currently tests are manually selected and put into objective C tests, in long term we may consider add some script to extract the libuv test cases and report if libuv added or removed test cases.

use the following command to run test:
./tools/bazel run //src/objective-c/tests/ThirdPartyTests/Libuv:LibuvTest

@drfloob
@dennycd

dennycd · 2022-01-14T21:33:03Z

thanks Hannah, it is great to see that we can now import and reuse libuv test suite ; ) Can you update the PR description with a few summary of

how iOS is reusing libuv test source
for ongoing maintenance (e.g. we bumped libuv version), what changes we need to make

dennycd · 2022-01-14T21:38:04Z

third_party/libuv_test.patch

+ static int connect_port = -1;
+
+-static int getsocknamecount = 0;
+static int getsocknamecount_tcp = 0;


I wonder why we need to separate the callback counting here (thus this patch), is there a difference on iOS test context that makes the vanilla libuv test invalid here ?

The static variable was used by multi test cases, it was OK since other languages use fork and count separately. But in iOS test, we have to run all tests in the same process, so we need separate variables to count separately.

dennycd · 2022-01-14T23:30:22Z

src/objective-c/tests/ThirdPartyTests/Libuv/LibuvTimerTests.m

+@implementation LibuvTimerTests
+
+- (void)testTimerAll {
+  for (task_entry_t* task = TASKS; task->main; task++) {


we will need a way to obtain the test result signals from these test by calling into XCTest's assertion methods (https://developer.apple.com/documentation/xctest?language=objc). Currently if we just run these it will always be reported as success (unless it crashed), thanks !

The ASSERT works. If a test case fails at any assert, the whole test will crash. It'll be reported as success, only when all assert in all test cases passed.

HannahShiSFB · 2022-01-24T19:45:10Z

thanks Hannah, it is great to see that we can now import and reuse libuv test suite ; ) Can you update the PR description with a few summary of

how iOS is reusing libuv test source

for ongoing maintenance (e.g. we bumped libuv version), what changes we need to make

added

dennycd · 2022-02-11T23:31:41Z

thanks for the update, all test looks good

drfloob

Sorry I didn't see this sooner. I'm glad to see you found a way to run libuv tests on iOS!

drfloob · 2022-02-12T03:12:25Z

third_party/libuv_test.patch

@@ -0,0 +1,56 @@
+diff --git a/test/test-timer.c b/test/test-timer.c


This looks like a general change to the libuv test code, nothing gRPC-specific. I don't see why we'd want to maintain patches for general improvements that they'd likely accept and maintain upstream. Patches are a maintenance burden best avoided, if possible. For example, if it does not apply cleanly onto the next libuv version, upgrading the libuv dependency now means we'd have to port a patch before we could do the upgrade.

Will send a PR to libuv after we finished adding other tests and collected all the patches needed.

Thanks. Please collect the patches separately, and remove them from this PR. If it was a patch for something that we absolutely need for libuv to function on iOS, the extra burden is justifiable. But this is not essential, and again, every patch we have to maintain adds a real maintenance burden for library upgrades.

Chatted w/ Hannah offline, we have a solution to run iOS test without patching libuv via ios_ui_test. This allows us to re-launch the app per test case via XCUIApplication.launch if needed in order to simulate the same fork run behavior as other platform.

unfortunately, re-launch ios ui app didn't work, it might because that it only restarts the ios ui toolkit part, not the entire process.
PS: ios ui test also runs super slow due to boot up the simulator.

If we cannot add patch in our build system, could we try upstream it to libuv and wait for them to merge first?

Another alternative maybe split LibuvTest target into smaller ones, though it'll make the project a bit messy.

Error log with ios ui test:

Assertion failed in external/com_github_libuv_libuv/test/test-timer.c on line 152: once_cb_called == 1
...
Failing tests:
Runner:
-[LibuvTimerTests testTimerStartTwice]

** TEST EXECUTE FAILED **

discussed offline, we will add back timer_start_twice test in a follow up PR pending the process relaunch fix

drfloob · 2022-02-12T03:16:18Z

third_party/libuv_test.patch

+   ASSERT(r == 0);
+
+-  ASSERT(once_cb_called == 1);
+  ASSERT(twice_cb_called == 1);


This change could be a bit misleading. The purpose of this test is that once_cb is called exactly once, even when started twice. Calling it twice_cb maybe implies that it's somehow different from once_cb, which it should not be.

It’s for test case timer_start_twice, which is the named similar to other xx_cb variables. Can we keep it for now and let the libuv team decide when I send them the PR?

drfloob · 2022-02-12T03:22:46Z

third_party/libuv.BUILD

+    deps = [
+        ":libuv"
+    ],
+    linkopts = select({


The same copts would be needed here as well, I think

comment addressed

drfloob · 2022-02-12T03:24:15Z

third_party/libuv.BUILD

+        "//conditions:default": [],
+    }),
+    visibility = [
+        "//visibility:public",


Does this need to be publicly visible?

Tried with "//src/objective-c/tests/ThirdPartyTests/Libuv:LibuvTestsLib” but didn’t work, any suggestions?

dennycd · 2022-02-25T23:42:19Z

Thanks for the update @HannahShiSFB . Let's remove libuv patch and split the PR with bazel only changes here. We can add iOS test setup in a follow up. Cheers

HannahShiSFB · 2022-03-10T17:56:19Z

Thanks for the update @HannahShiSFB . Let's remove libuv patch and split the PR with bazel only changes here. We can add iOS test setup in a follow up. Cheers

libuv patch removed.

* use libuv in third party * add ios libuv timer test * name changed correspondingly * format code * address comment * remove libuv patch

HannahShiSFB requested review from ctiller, gnossen, jtattermusch, nicolasnoble and veblush as code owners January 14, 2022 20:46

dennycd requested review from dennycd and removed request for gnossen, veblush, nicolasnoble, jtattermusch and ctiller January 14, 2022 20:47

dennycd added lang/ObjC platform/iOS release notes: no Indicates if PR should not be in release notes kokoro:run labels Jan 14, 2022

grpc-kokoro removed the kokoro:run label Jan 14, 2022

dennycd requested a review from drfloob January 14, 2022 20:49

dennycd added the kokoro:run label Jan 14, 2022

grpc-kokoro removed the kokoro:run label Jan 14, 2022

grpc-checks bot added the bloat/none label Jan 14, 2022

dennycd reviewed Jan 14, 2022

View reviewed changes

grpc-checks bot added the perf-change/none label Jan 14, 2022

dennycd reviewed Jan 14, 2022

View reviewed changes

HannahShiSFB force-pushed the ios_libuv_timer_test branch from eeb2c34 to 88a16f5 Compare January 24, 2022 19:06

HannahShiSFB force-pushed the ios_libuv_timer_test branch from 88a16f5 to 95e12df Compare January 27, 2022 16:13

dennycd added the kokoro:run label Jan 27, 2022

grpc-kokoro removed kokoro:run labels Jan 27, 2022

dennycd mentioned this pull request Jan 28, 2022

iOS test runner config to restart app process per test suite grpc/grpc-ios#51

Open

HannahShiSFB force-pushed the ios_libuv_timer_test branch from 95e12df to 1154ee3 Compare February 4, 2022 16:10

dennycd added the kokoro:run label Feb 4, 2022

grpc-kokoro removed the kokoro:run label Feb 4, 2022

HannahShiSFB force-pushed the ios_libuv_timer_test branch from 594599e to 09f7d81 Compare February 11, 2022 19:05

dennycd added the kokoro:run label Feb 11, 2022

grpc-kokoro removed the kokoro:run label Feb 11, 2022

dennycd approved these changes Feb 11, 2022

View reviewed changes

drfloob reviewed Feb 12, 2022

View reviewed changes

HannahShiSFB force-pushed the ios_libuv_timer_test branch from 09f7d81 to ecebdc3 Compare February 25, 2022 19:34

dennycd mentioned this pull request Feb 25, 2022

libuv timer test logic improvements grpc/grpc-ios#58

Open

HannahShiSFB added 6 commits March 10, 2022 09:54

use libuv in third party

0a4ff74

add ios libuv timer test

fc581a2

name changed correspondingly

3b335b9

format code

3066322

address comment

e233be0

remove libuv patch

861e18d

HannahShiSFB force-pushed the ios_libuv_timer_test branch from ecebdc3 to 861e18d Compare March 10, 2022 17:55

dennycd added the kokoro:run label Mar 10, 2022

grpc-kokoro removed kokoro:run labels Mar 10, 2022

dennycd merged commit 842ed8d into grpc:master Mar 16, 2022

copybara-service bot added the imported Specifies if the PR has been imported to the internal repository label Mar 16, 2022

HannahShiSFB mentioned this pull request Mar 19, 2022

separate the static variables by test cases libuv/libuv#3567

Merged

anicr7 pushed a commit to anicr7/grpc that referenced this pull request Apr 1, 2022

[Objc] add ios libuv timer test (grpc#28570)

7dba887

* use libuv in third party * add ios libuv timer test * name changed correspondingly * format code * address comment * remove libuv patch

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Objc] add ios libuv timer test #28570

[Objc] add ios libuv timer test #28570

HannahShiSFB commented Jan 14, 2022 •

edited

dennycd commented Jan 14, 2022

dennycd Jan 14, 2022

HannahShiSFB Jan 24, 2022

dennycd Jan 14, 2022

HannahShiSFB Jan 24, 2022

HannahShiSFB commented Jan 24, 2022

dennycd commented Feb 11, 2022

drfloob left a comment

drfloob Feb 12, 2022

HannahShiSFB Feb 25, 2022

drfloob Feb 25, 2022

dennycd Feb 25, 2022

HannahShiSFB Mar 2, 2022

dennycd Mar 16, 2022

drfloob Feb 12, 2022

HannahShiSFB Feb 25, 2022

drfloob Feb 12, 2022

HannahShiSFB Feb 25, 2022

drfloob Feb 12, 2022

HannahShiSFB Feb 25, 2022

dennycd commented Feb 25, 2022

HannahShiSFB commented Mar 10, 2022

		@@ -0,0 +1,56 @@
		diff --git a/test/test-timer.c b/test/test-timer.c

[Objc] add ios libuv timer test #28570

[Objc] add ios libuv timer test #28570

Conversation

HannahShiSFB commented Jan 14, 2022 • edited

dennycd commented Jan 14, 2022

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

HannahShiSFB commented Jan 24, 2022

dennycd commented Feb 11, 2022

drfloob 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

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

dennycd commented Feb 25, 2022

HannahShiSFB commented Mar 10, 2022

HannahShiSFB commented Jan 14, 2022 •

edited