Skip to content

isolate nlopt dependency further by moving inject.motion_service.go to its own package #4907

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

Merged
merged 4 commits into from
Apr 14, 2025
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion protoutils/messages.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import (
"strconv"

"github.com/golang/geo/r3"
protov1 "github.com/golang/protobuf/proto" //nolint:staticcheck
protov1 "github.com/golang/protobuf/proto"
Copy link
Member Author

Choose a reason for hiding this comment

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

I think this was added by the linter

commonpb "go.viam.com/api/common/v1"
robotpb "go.viam.com/api/robot/v1"
"go.viam.com/utils/protoutils"
Expand Down
8 changes: 0 additions & 8 deletions robot/impl/resource_manager_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,6 @@ import (
"go.viam.com/rdk/robot/framesystem"
"go.viam.com/rdk/robot/packages"
weboptions "go.viam.com/rdk/robot/web/options"
"go.viam.com/rdk/services/motion"
"go.viam.com/rdk/services/shell"
"go.viam.com/rdk/services/vision"
"go.viam.com/rdk/session"
Expand Down Expand Up @@ -462,13 +461,6 @@ func TestManagerAdd(t *testing.T) {
test.That(t, err, test.ShouldBeNil)
test.That(t, resource1, test.ShouldEqual, injectBoard)

injectMotionService := &inject.MotionService{}
Copy link
Member Author

Choose a reason for hiding this comment

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

This part of the test does not seem to add anything and means that anyone importing resource also needs to import nlopt by transitive dependencies (although the reason resource is importing this is unclear to me)

objectMResName := motion.Named("motion1")
manager.resources.AddNode(objectMResName, resource.NewConfiguredGraphNode(resource.Config{}, injectMotionService, unknownModel))
motionService, err := manager.ResourceByName(objectMResName)
test.That(t, err, test.ShouldBeNil)
test.That(t, motionService, test.ShouldEqual, injectMotionService)

injectVisionService := &inject.VisionService{}
injectVisionService.GetObjectPointCloudsFunc = func(
ctx context.Context,
Expand Down
4 changes: 2 additions & 2 deletions services/motion/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ import (
"go.viam.com/rdk/services/slam"
"go.viam.com/rdk/spatialmath"
"go.viam.com/rdk/testutils"
"go.viam.com/rdk/testutils/inject"
injectmotion "go.viam.com/rdk/testutils/inject/motion"
)

var (
Expand All @@ -43,7 +43,7 @@ func TestClient(t *testing.T) {
rpcServer, err := rpc.NewServer(logger, rpc.WithUnauthenticated())
test.That(t, err, test.ShouldBeNil)

injectMS := &inject.MotionService{}
injectMS := injectmotion.NewMotionService("test")
resources := map[resource.Name]motion.Service{
testMotionServiceName: injectMS,
}
Expand Down
3 changes: 2 additions & 1 deletion services/motion/motion_helpers_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,12 @@ import (
"go.viam.com/rdk/services/motion"
"go.viam.com/rdk/spatialmath"
"go.viam.com/rdk/testutils/inject"
injectmotion "go.viam.com/rdk/testutils/inject/motion"
)

func TestPollHistoryUntilSuccessOrError(t *testing.T) {
ctx := context.Background()
ms := inject.NewMotionService("my motion")
ms := injectmotion.NewMotionService("my motion")
t.Run("returns error if context is cancelled", func(t *testing.T) {
cancelledCtx, cancelFn := context.WithCancel(context.Background())
cancelFn()
Expand Down
18 changes: 9 additions & 9 deletions services/motion/server_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ import (
"go.viam.com/rdk/services/vision"
"go.viam.com/rdk/spatialmath"
"go.viam.com/rdk/testutils"
"go.viam.com/rdk/testutils/inject"
injectmotion "go.viam.com/rdk/testutils/inject/motion"
)

func newServer(resources map[resource.Name]motion.Service) (pb.MotionServiceServer, error) {
Expand All @@ -53,7 +53,7 @@ func TestServerMove(t *testing.T) {
test.That(t, err, test.ShouldBeError, errors.New("resource \"rdk:service:motion/motion1\" not found"))

// error
injectMS := &inject.MotionService{}
injectMS := injectmotion.NewMotionService("test")
resources = map[resource.Name]motion.Service{
testMotionServiceName: injectMS,
}
Expand All @@ -76,7 +76,7 @@ func TestServerMove(t *testing.T) {
test.That(t, resp.GetSuccess(), test.ShouldBeTrue)

// Multiple Servies names Valid
injectMS = &inject.MotionService{}
injectMS = injectmotion.NewMotionService("test")
resources = map[resource.Name]motion.Service{
testMotionServiceName: injectMS,
testMotionServiceName2: injectMS,
Expand All @@ -97,7 +97,7 @@ func TestServerMove(t *testing.T) {
}

func TestServerMoveOnGlobe(t *testing.T) {
injectMS := &inject.MotionService{}
injectMS := injectmotion.NewMotionService("test")
resources := map[resource.Name]motion.Service{
testMotionServiceName: injectMS,
}
Expand Down Expand Up @@ -279,7 +279,7 @@ func TestServerMoveOnGlobe(t *testing.T) {
}

func TestServerMoveOnMap(t *testing.T) {
injectMS := &inject.MotionService{}
injectMS := injectmotion.NewMotionService("test")
resources := map[resource.Name]motion.Service{
testMotionServiceName: injectMS,
}
Expand Down Expand Up @@ -450,7 +450,7 @@ func TestServerMoveOnMap(t *testing.T) {
}

func TestServerStopPlan(t *testing.T) {
injectMS := &inject.MotionService{}
injectMS := injectmotion.NewMotionService("test")
resources := map[resource.Name]motion.Service{
testMotionServiceName: injectMS,
}
Expand Down Expand Up @@ -515,7 +515,7 @@ func TestServerStopPlan(t *testing.T) {
}

func TestServerListPlanStatuses(t *testing.T) {
injectMS := &inject.MotionService{}
injectMS := injectmotion.NewMotionService("test")
resources := map[resource.Name]motion.Service{
testMotionServiceName: injectMS,
}
Expand Down Expand Up @@ -621,7 +621,7 @@ func TestServerListPlanStatuses(t *testing.T) {
}

func TestServerGetPlan(t *testing.T) {
injectMS := &inject.MotionService{}
injectMS := injectmotion.NewMotionService("test")
resources := map[resource.Name]motion.Service{
testMotionServiceName: injectMS,
}
Expand Down Expand Up @@ -751,7 +751,7 @@ func TestServerGetPlan(t *testing.T) {

func TestServerDoCommand(t *testing.T) {
resourceMap := map[resource.Name]motion.Service{
testMotionServiceName: &inject.MotionService{
testMotionServiceName: &injectmotion.MotionService{
DoCommandFunc: testutils.EchoFunc,
},
}
Expand Down
25 changes: 13 additions & 12 deletions services/navigation/builtin/builtin_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,12 +37,13 @@ import (
_ "go.viam.com/rdk/services/vision/colordetector"
"go.viam.com/rdk/spatialmath"
"go.viam.com/rdk/testutils/inject"
injectmotion "go.viam.com/rdk/testutils/inject/motion"
viz "go.viam.com/rdk/vision"
)

type startWaypointState struct {
ns navigation.Service
injectMS *inject.MotionService
injectMS *injectmotion.MotionService
base base.Base
movementSensor *inject.MovementSensor
closeFunc func()
Expand Down Expand Up @@ -230,7 +231,7 @@ func TestNew(t *testing.T) {
}
deps := resource.Dependencies{
resource.NewName(base.API, "base"): inject.NewBase("new_base"),
resource.NewName(motion.API, "builtin"): inject.NewMotionService("new_motion"),
resource.NewName(motion.API, "builtin"): injectmotion.NewMotionService("new_motion"),
resource.NewName(movementsensor.API, "movement_sensor"): inject.NewMovementSensor("movement_sensor"),
}

Expand All @@ -252,7 +253,7 @@ func TestNew(t *testing.T) {
}
deps := resource.Dependencies{
resource.NewName(base.API, "base"): inject.NewBase("new_base"),
resource.NewName(motion.API, "builtin"): inject.NewMotionService("new_motion"),
resource.NewName(motion.API, "builtin"): injectmotion.NewMotionService("new_motion"),
resource.NewName(movementsensor.API, "movement_sensor"): inject.NewMovementSensor("movement_sensor"),
}

Expand Down Expand Up @@ -285,7 +286,7 @@ func TestNew(t *testing.T) {
deps := resource.Dependencies{
resource.NewName(base.API, "base"): &inject.Base{},
resource.NewName(camera.API, "camera"): inject.NewCamera("camera"),
resource.NewName(motion.API, "builtin"): inject.NewMotionService("motion"),
resource.NewName(motion.API, "builtin"): injectmotion.NewMotionService("motion"),
resource.NewName(vision.API, "vision"): inject.NewVisionService("vision"),
}

Expand Down Expand Up @@ -319,7 +320,7 @@ func TestNew(t *testing.T) {
deps := resource.Dependencies{
resource.NewName(base.API, "base"): &inject.Base{},
resource.NewName(camera.API, "camera"): inject.NewCamera("camera"),
resource.NewName(motion.API, "builtin"): inject.NewMotionService("motion"),
resource.NewName(motion.API, "builtin"): injectmotion.NewMotionService("motion"),
resource.NewName(vision.API, "vision"): inject.NewVisionService("vision"),
}

Expand Down Expand Up @@ -361,7 +362,7 @@ func TestNew(t *testing.T) {
}
deps := resource.Dependencies{
resource.NewName(base.API, "base"): &inject.Base{},
resource.NewName(motion.API, "builtin"): inject.NewMotionService("motion"),
resource.NewName(motion.API, "builtin"): injectmotion.NewMotionService("motion"),
}

err := svc.Reconfigure(ctx, deps, resource.Config{ConvertedAttributes: cfg})
Expand All @@ -381,7 +382,7 @@ func TestNew(t *testing.T) {
}
deps := resource.Dependencies{
resource.NewName(base.API, "base"): &inject.Base{},
resource.NewName(motion.API, "builtin"): inject.NewMotionService("motion"),
resource.NewName(motion.API, "builtin"): injectmotion.NewMotionService("motion"),
resource.NewName(camera.API, "camera"): inject.NewCamera("camera"),
resource.NewName(movementsensor.API, "movement_sensor"): inject.NewMovementSensor("movement_sensor"),
}
Expand All @@ -403,7 +404,7 @@ func TestNew(t *testing.T) {
}
deps := resource.Dependencies{
resource.NewName(base.API, "base"): &inject.Base{},
resource.NewName(motion.API, "builtin"): inject.NewMotionService("motion"),
resource.NewName(motion.API, "builtin"): injectmotion.NewMotionService("motion"),
resource.NewName(vision.API, "vision"): inject.NewVisionService("vision"),
resource.NewName(movementsensor.API, "movement_sensor"): inject.NewMovementSensor("movement_sensor"),
}
Expand All @@ -425,7 +426,7 @@ func TestNew(t *testing.T) {
}
deps := resource.Dependencies{
resource.NewName(base.API, "base"): &inject.Base{},
resource.NewName(motion.API, "builtin"): inject.NewMotionService("motion"),
resource.NewName(motion.API, "builtin"): injectmotion.NewMotionService("motion"),
resource.NewName(vision.API, "vision"): inject.NewVisionService("vision"),
resource.NewName(camera.API, "camera"): inject.NewCamera("camera"),
resource.NewName(movementsensor.API, "movement_sensor"): inject.NewMovementSensor("movement_sensor"),
Expand Down Expand Up @@ -633,7 +634,7 @@ func setupStartWaypoint(ctx context.Context, t *testing.T, logger logging.Logger
},
},
}
injectMS := inject.NewMotionService("test_motion")
injectMS := injectmotion.NewMotionService("test_motion")
deps := resource.Dependencies{
injectMS.Name(): injectMS,
fakeBase.Name(): fakeBase,
Expand Down Expand Up @@ -684,7 +685,7 @@ func setupStartWaypointExplore(ctx context.Context, t *testing.T, logger logging
},
},
}
injectMS := inject.NewMotionService("test_motion")
injectMS := injectmotion.NewMotionService("test_motion")
deps := resource.Dependencies{
injectMS.Name(): injectMS,
fakeBase.Name(): fakeBase,
Expand Down Expand Up @@ -1722,7 +1723,7 @@ func TestGetObstacles(t *testing.T) {
)
test.That(t, err, test.ShouldBeNil)

injectMS := inject.NewMotionService("test_motion")
injectMS := injectmotion.NewMotionService("test_motion")
injectedVis := inject.NewVisionService("test_vision")
injectMovementSensor := inject.NewMovementSensor("test_movement")
injectedCam := inject.NewCamera("test_camera")
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
// Package inject separates the injected motion service from the rest of the injected packages to isolate an NLopt dependency.
package inject

import (
Expand Down
Loading