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

[RSDK-9149] Fix Logger Test Fails #4723

Merged
merged 18 commits into from
Jan 22, 2025
Merged

Conversation

bashar-515
Copy link
Member

@bashar-515 bashar-515 commented Jan 16, 2025

This pull request is causing some RDK tests to fail due to a) unexpected - unexpected by the tests, expected by us - log messages showing up and b) a data race (likely due to a message being logged in a go routine caused by a test that has already completed/returned). This PR fixes those failures by allowing for the graceful stopping of the web servers used in the tests that are failing.

@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jan 16, 2025
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jan 16, 2025
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jan 16, 2025
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jan 17, 2025
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jan 21, 2025
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jan 21, 2025
@@ -73,6 +73,8 @@ type Options struct {
WebRTCOnPeerRemoved func(pc *webrtc.PeerConnection)

DisableMulticastDNS bool

WaitForHandlers bool
Copy link
Member

Choose a reason for hiding this comment

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

add a comment explaining what this comment does

@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jan 21, 2025
@bashar-515 bashar-515 requested a review from cheukt January 21, 2025 18:01
@viambot viambot added the safe to test This pull request is marked safe to test from a trusted zone label Jan 21, 2025
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jan 21, 2025
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jan 21, 2025
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jan 21, 2025
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jan 21, 2025
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jan 21, 2025
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jan 21, 2025
@viambot viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jan 21, 2025
@bashar-515 bashar-515 changed the title [RSDK-9149] Fix Logger Testing Data Race [RSDK-9149] Fix Logger Test Fails Jan 21, 2025
Copy link
Member Author

@bashar-515 bashar-515 Jan 21, 2025

Choose a reason for hiding this comment

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

I completely removed the two failing test bc it didn't seem right to assign a seemingly arbitrary number as the expected number of logs. Plus, the number varies by platform. So, I think it's better for the tests to not be included to say that we do now expect some number of error logs here.

@bashar-515 bashar-515 marked this pull request as ready for review January 21, 2025 22:17
Copy link
Member

@dgottlieb dgottlieb left a comment

Choose a reason for hiding this comment

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

I don't have an opinion here, I'm fine with these changes.

I defer to @cheukt

Copy link
Member

@cheukt cheukt left a comment

Choose a reason for hiding this comment

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

LGTM, removals seem fine

@bashar-515 bashar-515 merged commit 36db2d2 into viamrobotics:main Jan 22, 2025
16 checks passed
@bashar-515 bashar-515 deleted the RSDK-9149 branch January 22, 2025 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
safe to test This pull request is marked safe to test from a trusted zone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants