Skip to content

Backport iOS syslog broadcast robustness fixes to Android logcat #227

Description

@Dor-bl

Context

PR #226 (iOS syslog broadcast) and its upstream counterpart appium#1096 applied several robustness and test-hygiene fixes in response to review feedback. The iOS IOSDriver / SyslogBroadcastTests were originally a direct mirror of the Android AndroidDriver logcat implementation (merged in appium#969), so the same issues exist on the Android side and were intentionally not touched in that PR to keep its scope to iOS.

This issue tracks backporting the equivalent fixes to Android so the two platforms stay consistent.

Changes to backport

AndroidDriver.cs:

  • StartLogcatBroadcast(host, port): if the WebSocket ConnectAsync fails, the server-side mobile: startLogsBroadcast has already run, leaving an orphaned broadcast on the Appium server. Stop the broadcast (best-effort) on connection failure, then rethrow.
  • StopLogcatBroadcast(): wrap mobile: stopLogsBroadcast in a try/finally so DisconnectAsync always runs even if the stop command throws.
  • Build the WebSocket endpoint via UriBuilder instead of $"ws://{host}:{port}/..." string interpolation, so IPv6 literal hosts (e.g. ::1) are bracketed correctly rather than throwing UriFormatException.

test/integration/Android/Session/Logs/LogcatBroadcastTests.cs:

  • Replace blocking SemaphoreSlim.Wait(...) with await WaitAsync(...) in the async tests.
  • Replace Thread.Sleep(...) with await Task.Delay(...).
  • CanHandleErrorsGracefully: catch WebDriverException instead of AggregateException. Awaited StringWebSocketClient.ConnectAsync surfaces connection failures as WebDriverException directly (not wrapped in AggregateException), so the current catch clause is effectively dead code.
  • CanHandleErrorsGracefully: remove the declared-but-never-awaited errorSemaphore and its Release() call.
  • Message-received signals use bounded SemaphoreSlim maxCounts (1 in VerifyLogcatListenerCanBeAssigned, 10 in CanAddAndRemoveMultipleListeners) while the handler calls Release() per message. A burst of logcat messages can exceed the max and throw SemaphoreFullException, making the tests flaky. Use int.MaxValue as the max.
  • CanAddAndRemoveMultipleListeners: fix the misleading "both listeners should be called" comment to match the assertion (the semaphore wakes on the first listener, so the test only asserts at least one invocation).
  • CanAddAndRemoveMultipleListeners: add a short drain delay after RemoveAllLogcatListeners() and before resetting the counter. RemoveAll... races with in-flight dispatch — a message already read from the socket before removal can still invoke the previously captured delegate, flipping the post-removal EqualTo(0) assertion.

Shared-client issue to fix (affects both platforms)

  • StringWebSocketClient.DisconnectInternalAsync only completes the close handshake and fires _disconnected handlers when State == Open. On a server-initiated close, ReceiveMessagesAsync calls DisconnectAsync() while the state is CloseReceived, so the handshake and disconnection handlers are skipped. Fix is non-trivial: the server-close path calls DisconnectAsync from within the receive loop, so naively broadening the state check would make DisconnectInternalAsync await its own _receiveTask and deadlock. Needs a dedicated fix with tests (raised by Copilot on feat: Add WebSocket-based syslog broadcast support for iOS appium/dotnet-client#1096).

Already fixed for both platforms (not a backport item)

Reference

See the corresponding iOS changes and reviewer discussion in PR #226 and appium#1096.

Optional / follow-up

  • Consider deriving the WebSocket host/port from the test fixture's Appium server URI (instead of the hardcoded 127.0.0.1:4723) so the broadcast tests also work against a remote server — ideally applied to both the Android and iOS suites together.
  • Separate CI-stability concern (not specific to broadcast tests): the Android integration job intermittently flakes — ActionsChainsTest (StaleElementReferenceException) and AppStringsTest (OneTimeSetUp new-session HTTP timeout after 10s). Worth investigating a longer session-start timeout and/or a retry, tracked separately if it persists.

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementNew feature or request

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions