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

(powersensor/sensor) Guard against nil responses in rdk client/servers #4621

Merged
merged 3 commits into from
Dec 17, 2024

Conversation

martha-johnston
Copy link
Contributor

@martha-johnston martha-johnston commented Dec 11, 2024

went through power sensor, sensor, and servo server code to see what had the possibility to be returned as nil, and added checks to return errors in that case. also added tests for each nil failure. there were no changes for servo.

added necessary tests to client test file to ensure that the server changes resulted in the same returned error, so the checks didn't need to be added in the client files as well.

work for other components are in other PRs (#4618 & #4620)

@viambot viambot added the safe to test This pull request is marked safe to test from a trusted zone label Dec 11, 2024
@martha-johnston martha-johnston requested review from a team and JohnN193 and removed request for a team December 11, 2024 22:08
@martha-johnston martha-johnston marked this pull request as ready for review December 11, 2024 22:08
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 [q], did we not need a nil check in the client as well?

@martha-johnston
Copy link
Contributor Author

LGTM but [q], did we not need a nil check in the client as well?

honestly I am still not 100% sure. I originally thought yes, but I ended up writing the client tests before actually making the changes in client.go, and the tests passed with the expected errors returned from the functions in server.go? haven't fully wrapped my mind around how client and server interact in every case.

the case I'm curious about is if you call the client code directly through an sdk, would the errors not be surfaced? and if so, why are the client tests not set up to replicate that case?

@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 Dec 12, 2024
@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 Dec 17, 2024
@martha-johnston martha-johnston merged commit cb29ca7 into viamrobotics:main Dec 17, 2024
16 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