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.

Sign up for GitHub

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

Jump to bottom

APP-7497: Add Button client, server, and fake model #4740

Merged
merged 5 commits into from
Jan 29, 2025
Merged

APP-7497: Add Button client, server, and fake model #4740

merged 5 commits into from
Jan 29, 2025

Conversation

ethanlookpotts
Copy link
Member

@ethanlookpotts ethanlookpotts commented Jan 24, 2025 •
edited
Loading

This adds the button client and server, and registers a button fake model.

Here's a button component properly configured in app:
image

  1. APP-7497: Add Button and Switch APIs api#619
  2. APP-7497: Add Button client, server, and fake model #4740
  3. APP-7497: Add Switch client, server, and fake models #4741

Change log

I used Cursor quite a bit here:

  • Bump go.viam.com/api version
  • Add Button SDK helpers
  • Add client and tests
  • Add server and tests
  • Add inject testutil
  • Add fake button implementation and register it
    • This implementation just logs a message when pushed

Review requests

  • Code review
  • Try a config with the button fake - does it work as expected?

@viambot viambot added the safe to test This pull request is marked safe to test from a trusted zone label Jan 24, 2025
@ethanlookpotts ethanlookpotts force-pushed the APP-7497/ethanlookpotts/button branch from 87741a8 to 6b4f06f Compare January 24, 2025 18:50
@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 Jan 24, 2025
@ethanlookpotts ethanlookpotts mentioned this pull request Jan 24, 2025
Merged
@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 Jan 27, 2025
@ethanlookpotts ethanlookpotts force-pushed the APP-7497/ethanlookpotts/button branch from d1ec868 to d62aae2 Compare January 27, 2025 17:45
@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 Jan 27, 2025
randhid
randhid reviewed Jan 27, 2025
View reviewed changes
components/button/fake/button.go Outdated
Comment on lines 15 to 18
// Config is the config for a fake button.
type Config struct {
resource.TriviallyValidateConfig
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Config is the config for a fake button.
type Config struct {
resource.TriviallyValidateConfig
}

With resource.NoNativeConfig below

randhid
randhid requested changes Jan 27, 2025
View reviewed changes
components/button/fake/button.go Outdated Show resolved Hide resolved
components/button/fake/button.go Show resolved Hide resolved
components/button/fake/button.go Outdated Show resolved Hide resolved
components/button/fake/button.go Outdated Show resolved Hide resolved
testutils/inject/button.go Outdated Show resolved Hide resolved
components/button/button.go Show resolved Hide resolved
@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 Jan 27, 2025
randhid
randhid reviewed Jan 29, 2025
View reviewed changes
components/button/server.go Outdated
func (s *serviceServer) DoCommand(ctx context.Context,
req *commonpb.DoCommandRequest,
) (*commonpb.DoCommandResponse, error) {
gripper, err := s.coll.Resource(req.GetName())
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
gripper, err := s.coll.Resource(req.GetName())
button, err := s.coll.Resource(req.GetName())

Copy link
Member Author

Choose a reason for hiding this comment

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

1ff98bc

components/button/server.go Outdated
if err != nil {
return nil, err
}
return protoutils.DoFromResourceServer(ctx, gripper, req)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return protoutils.DoFromResourceServer(ctx, gripper, req)
return protoutils.DoFromResourceServer(ctx, button, req)

Copy link
Member Author

Choose a reason for hiding this comment

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

1ff98bc

randhid
randhid reviewed Jan 29, 2025
View reviewed changes
components/button/server_test.go
t.Run("push", func(t *testing.T) {
_, err := buttonServer.Push(context.Background(), &pb.PushRequest{Name: missingButtonName})
test.That(t, err, test.ShouldNotBeNil)
test.That(t, err.Error(), test.ShouldContainSubstring, errButtonNotFound.Error())
Copy link
Member

Choose a reason for hiding this comment

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

[super nit] could you compare the errors directly? don't try to do this in the client as it wraps it was a "grpc error blah blah", but I prefer comparing the errors directly in server code

rdk/components/movementsensor/server_test.go

Line 134 in 93eea2e

test.That(t, err, test.ShouldBeError, errLinearVelocity)

Copy link
Member Author

Choose a reason for hiding this comment

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

1ff98bc

I could only do this for the push error. For the missing errors, the error is wrapped with the resource's name so it does not exactly match the error.

randhid
randhid reviewed Jan 29, 2025
View reviewed changes
components/button/button_test.go Outdated
@@ -0,0 +1,33 @@
package button_test
Copy link
Member

Choose a reason for hiding this comment

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

Move to fake, it's testing that package, the helpers in the interface package are tested in the resource graph.

Copy link
Member Author

Choose a reason for hiding this comment

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

1ff98bc

@randhid
Copy link
Member

randhid commented Jan 29, 2025

Could you add a follow up ticket for me to "add fake button and switch to fake.json in rdk" - do NOT do it yourself, it will open up a can of worms in robot and resource graph.

@ethanlookpotts
Copy link
Member Author

Could you add a follow up ticket for me to "add fake button and switch to fake.json in rdk" - do NOT do it yourself, it will open up a can of worms in robot and resource graph.

https://viam.atlassian.net/browse/RSDK-9853

@ethanlookpotts ethanlookpotts force-pushed the APP-7497/ethanlookpotts/button branch from d52ab1b to 1ff98bc Compare January 29, 2025 15:12
@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 Jan 29, 2025
@ethanlookpotts ethanlookpotts merged commit a891617 into main Jan 29, 2025
16 checks passed
@ethanlookpotts ethanlookpotts deleted the APP-7497/ethanlookpotts/button branch January 29, 2025 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@randhid randhid randhid approved these changes

@stevebriskin stevebriskin Awaiting requested review from stevebriskin

Assignees
No one assigned
Labels
safe to test This pull request is marked safe to test from a trusted zone
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ethanlookpotts @randhid @viambot