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.

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

Improve test.remote reliability #12573

Merged
merged 5 commits into from
Jan 16, 2024
Merged

Conversation

maloel
Copy link
Contributor

@maloel maloel commented Jan 15, 2024

During development I noticed many cases, even locally, of tests failing with output like this:

[C  ] -D- 11:55:48.216 client: +writer (server.903) publishing 'realsense/device-info' [realdds::topics::raw::flexible reliable] (dds-participant.cpp:144 [20052])
___rea[  S] -D- 11:55:48.216 broadcasting discovery notifications (dds-notification-server.cpp:161 [16444])
[  S] dy
[  S] -D- 11:55:48.278 server: broadcasting (dds-device-broadcaster.cpp:81 [7116])

test.remote uses a specific string ___ready\n to signify to the client that it's in a ready state.
Notice how, in the above, this string is split by a line that's output from another thread! This breaks it up, the client doesn't realize it's ready, and times out - failing the test.

This attempts to identify such cases and deal with them. So far it's worked in my testing. Hopefully it will help in GHA, too.

Likewise:

-I- Test passed
-D- waiting for remote to finish...
-D- 12:31:51.518 client: -writer (.203) publishing 'server/device/control' (dds-participant.cpp:151 [140585218340608])
terminate called after throwing an instance of 'std::system_error'
  what():  Resource deadlock avoided
-D-     test took 1.2171788215637207 seconds
-E- test-dds-control-reply: exited with non-zero value (-6)

Googling this shows this is the result of "when a thread tries to join itself." Try to avoid this, too...

Added some minor DDS stuff, in own commits that should be self-contained. No new functionality (sample-lost callbacks don't work yet because of an eProsima bug -- I reported it and they fixed it).

@maloel maloel added the ci CI-related: does not change library behavior label Jan 15, 2024
@maloel maloel requested a review from OhadMeir January 15, 2024 12:42
Copy link
Contributor

@OhadMeir OhadMeir left a comment

Choose a reason for hiding this comment

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

LGTM

@maloel maloel merged commit 84d89d0 into IntelRealSense:development Jan 16, 2024
16 checks passed
@maloel maloel deleted the test-remote branch January 16, 2024 06:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci CI-related: does not change library behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants