RSDK-3950 - call into cgo for getPosition #188

kim-mishra · 2023-07-11T17:59:15Z

If the modularizationV2Enabled feature flag is true, call into the new cartofacade API for getPosition
Write unit tests to ensure new conversion returns the same values as the old conversions would have

viam-cartographer.go

nicksanford · 2023-07-11T18:30:23Z

viam-cartographer_internal_test.go

+	var inputPose commonv1.Pose
+	var inputQuat map[string]interface{}
+
+	t.Run("successful client", func(t *testing.T) {


what is the client? Can we just remove this t.Run so the rest of them are at the top level?

nicksanford · 2023-07-11T18:30:45Z

viam-cartographer_internal_test.go

+			test.That(t, pose, test.ShouldResemble, expectedPose)
+		})
+
+		t.Run("non origin pose success", func(t *testing.T) {


can we test the error case please?

nicksanford · 2023-07-11T18:31:31Z

viam-cartographer_internal_test.go

@@ -243,6 +244,75 @@ func TestGetPositionEndpoint(t *testing.T) {
 	})
 }

+//nolint:dupl


is this still necessary if we remove 257?

With 257 removed, I still get: 284-310 lines are duplicate of viam-cartographer_internal_test.go:256-282

can you please refactor it to not hit this lint rule by making a helper function?

nicksanford · 2023-07-11T18:35:47Z

viam-cartographer.go

+func (cartoSvc *cartographerService) getPositionModularizationV2(ctx context.Context) (spatialmath.Pose, string, error) {
+	pos, err := cartoSvc.cartofacade.GetPosition(ctx, cartoSvc.cartoFacadeTimeout)
+	if err != nil {
+		return nil, "", err


can we please test this error case?

nicksanford · 2023-07-11T18:36:17Z

viam-cartographer_internal_test.go

+
+		inputPose = commonv1.Pose{X: 0, Y: 0, Z: 0, OX: 0, OY: 0, OZ: 1, Theta: 0}
+		inputQuat = map[string]interface{}{"real": 1.0, "imag": 0.0, "jmag": 0.0, "kmag": 0.0}
+		pose, _, err := svc.GetPosition(context.Background())


why are we not testing the component reference?

I will add that back in, I got confused about using this after some of the changes from component reference to sensors[0], but this will still be needed after full mod in the case of multiple sensors, right?

yes correct

nicksanford · 2023-07-11T18:36:23Z

viam-cartographer_internal_test.go

+
+		inputPose = commonv1.Pose{X: 5, Y: 5, Z: 5, OX: 0, OY: 0, OZ: 1, Theta: 0}
+		inputQuat = map[string]interface{}{"real": 1.0, "imag": 1.0, "jmag": 0.0, "kmag": 0.0}
+		pose, _, err := svc.GetPosition(context.Background())


why are we not testing the component reference?

nicksanford · 2023-07-11T18:38:40Z

viam-cartographer_internal_test.go

@@ -243,6 +244,73 @@ func TestGetPositionEndpoint(t *testing.T) {
 	})
 }

+//nolint:dupl
+func TestGetPositionModularizationV2Endpoint(t *testing.T) {


We need a test for "empty component reference success"

nicksanford · 2023-07-11T18:39:30Z

viam-cartographer_internal_test.go

+
+		inputPose = commonv1.Pose{X: 0, Y: 0, Z: 0, OX: 0, OY: 0, OZ: 1, Theta: 0}
+		inputQuat = map[string]interface{}{"real": 1.0, "imag": 0.0, "jmag": 0.0, "kmag": 0.0}
+		pose, _, err := svc.GetPosition(context.Background())


why are we not testing the component reference?

getPosition and tests

05d6255

viambot added the safe to test This pull request is marked safe to test from a trusted zone label Jul 11, 2023

kim-mishra changed the title ~~getPosition and tests~~ RSDK-3950 - call into cgo for getPosition Jul 11, 2023

lint

d28e78e