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

Clarify Robot Window HTML Test #6755

Open
wants to merge 17 commits into
base: develop
Choose a base branch
from

Conversation

CoolSpy3
Copy link
Contributor

@CoolSpy3 CoolSpy3 commented Jan 12, 2025

Description
This PR fixes a potential race condition in the robot_window_html test. The linked issue mentioned that this test was failing. However, more interestingly, the recent-runs of this test have been passing (since ~Dec 17). No changes were made to the develop branch during this time.

My guess is that this failure was caused by the test not exiting the wb_robot_wwi_receive_text loop after calling wb_robot_wwi_send_text. If the robot window returns the response quickly, it is consumed by the first loop, causing the second loop to hang indefinitely. This would explain the intermittent failures.

This PR forces the loop to break after the send statement, fixing this case. This should hopefully also fix the test on 22.04.

It should be noted that, for me, the test passes normally, but fails in a headless environment (presumably due to the web browser not starting), so that could be contributing to the difficulties in the CI environment. My hope is that because the test has passed before, it should be functional here.

UPDATE: My theory appears to have been wrong. The failure still occurs here (although why it passes on develop remains a mystery. I managed to fix my local build in headless mode by switching off of the default snap install of Firefox, however, that same fix doesn't appear to work here. (In fact, the logs indicate that Firefox is not installed through snap on GitHub runners.)

UPDATE 2: Alright, it looks like a runner update was pushed at around the same time this test started passing on develop, so I'm going to guess that that's the culprit. However, it also appears that these changes still fail on master, but pass here. Perhaps it's due to the ongoing work to make develop compatible with 24.04? In any case, I've backed out most of my changes here. What remains is the following:

  1. The way the original test was written was somewhat confusing for me. I've rewritten some of the logic to hopefully make it clearer.
    • The test message also referenced complete_test, which briefly led me down a rabbit hole of investigating the ROS tests. I've fixed the message to reflect the correct information.
  2. Early on, I started running this test on Ubuntu 22.04. It's been consistently passing there, so I'll assume that whatever problem had led to it being disabled no longer exists. Thus, this PR reenables it.

Related Issues
This pull-request fixes issue #6727.

@CoolSpy3 CoolSpy3 added test suite issue Test failure in the test suite CI test suite Start the test suite labels Jan 12, 2025
@CoolSpy3 CoolSpy3 changed the title Fix race condition in robot window html test Fix Race Condition in Robot Window HTML Test Jan 12, 2025
@CoolSpy3 CoolSpy3 changed the base branch from master to develop January 12, 2025 18:10
@CoolSpy3 CoolSpy3 force-pushed the fix-race-condition-in-robot-window-html-test branch from 41c62a6 to 1abb36a Compare January 12, 2025 18:16
@CoolSpy3 CoolSpy3 added test suite Start the test suite and removed test suite Start the test suite labels Jan 12, 2025
@CoolSpy3 CoolSpy3 added the test distribution Start the distribution test label Jan 12, 2025
@CoolSpy3 CoolSpy3 changed the title Fix Race Condition in Robot Window HTML Test Clarify Robot Window HTML Test Jan 12, 2025
@CoolSpy3 CoolSpy3 marked this pull request as ready for review January 13, 2025 00:27
@CoolSpy3 CoolSpy3 requested a review from a team as a code owner January 13, 2025 00:27
@CoolSpy3 CoolSpy3 removed the test distribution Start the distribution test label Jan 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test suite issue Test failure in the test suite CI test suite Start the test suite
Development

Successfully merging this pull request may close these issues.

1 participant