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-8824: Test Datarace: go.viam.com/rdk/components/base/sensorcontrolled.TestSensorBaseWithVelocitiesSensor #4395

Merged
merged 1 commit into from
Sep 30, 2024

Conversation

martha-johnston
Copy link
Contributor

after further investigation the issue was in the Next() function of various blocks. I added a lock to each, but I'm wondering if this will cause issues. @JohnN193 do you remember why we haven't done this yet? I did a quick test with tuning and behavior of controlled components with these changes and didn't see any issues

@viambot viambot added the safe to test This pull request is marked safe to test from a trusted zone label Sep 25, 2024
@JohnN193
Copy link
Member

I don't think there was a particular reason, I tried to ask why we didn't have locks in the past and never got a response back. Chose not to add locks because we had a ticket to fix the controls/motor tests so figured all the other blocks could happen then.

Copy link
Member

@JohnN193 JohnN193 left a comment

Choose a reason for hiding this comment

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

LGTM, but could you check the behavior of the skipped tests in sensorcontrolled_test.go and controlled motor?

@martha-johnston
Copy link
Contributor Author

LGTM, but could you check the behavior of the skipped tests in sensorcontrolled_test.go and controlled motor?

they all still fail

@martha-johnston
Copy link
Contributor Author

going to run more extensive hardware tests tomorrow to make sure this doesn't break anything.

@martha-johnston martha-johnston merged commit 24b4ba4 into viamrobotics:main Sep 30, 2024
19 checks passed
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.

3 participants