RDSK-9311: (gripper/input-controller) Guard against nil responses in rdk client/servers #4620
Conversation
viambot
added
the
safe to test
martha-johnston
changed the title
martha-johnston
changed the title
viambot
added
safe to test
viambot
added
safe to test
martha-johnston
requested review from
a team and
oliviamiller
and removed request for
a team
viambot
added
safe to test
| @@ -145,6 +148,9 @@ func TestClient(t *testing.T) { | |||
| test.That(t, err, test.ShouldNotBeNil) | |||
| test.That(t, err.Error(), test.ShouldContainSubstring, errStopUnimplemented.Error()) | |||
|
|
|||
| _, err = client2.Geometries(context.Background(), extra) | |||
| test.That(t, err.Error(), test.ShouldContainSubstring, gripper.ErrGeometriesNil(failGripperName).Error()) | |||
There was a problem hiding this comment.
wouldErrors.Is work here? I think we are trying to avoid using substring for errors test when possible
There was a problem hiding this comment.
the problem is that it adds "rpc error: code = Unknown desc = " before the actual error, so figured it was easier to use substring. in the server code it doesn't add that string, so you can just use test.ShouldBeError
There was a problem hiding this comment.
although I guess I didn't try Errors.Is perhaps that's easier, I've never seen that in the test.That() format, I'll try it out
There was a problem hiding this comment.
makes sense thanks for explaining! I meant test.ShouldBeError so prob fine to leave as is
|
|
||
| _, err = client2.Events(context.Background(), map[string]interface{}{}) | ||
| test.That(t, err, test.ShouldNotBeNil) | ||
| test.That(t, err.Error(), test.ShouldContainSubstring, errEventsFailed.Error()) | ||
| test.That(t, err.Error(), test.ShouldContainSubstring, input.ErrEventsNil(failInputControllerName).Error()) |
There was a problem hiding this comment.
Should we still be testing the previous errors or is it not needed?
There was a problem hiding this comment.
it's not needed because errControlsFailed is just an error that was defined in this package and was only returned from the injected input controller. now we are testing if the real function returns the expected error
went through encoder, gripper, input controller, motor, and posetracker 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 encoder, motor, or pose tracker.
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.
still need to do this for the other components, but want to break up the work into smaller PRs (#4618 & #4621)