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

Improve fake server interface #3698

Merged
merged 13 commits into from
Mar 15, 2024

Conversation

maximpertsov
Copy link
Contributor

@maximpertsov maximpertsov commented Mar 15, 2024

Makes start-up test checks internal to the NewFakeCloudServer test helper. These checks are necessary but distracting from the tests in which this utility is used.

More generally follows this proposed pattern for test helpers

Never return errors. Just pass in *testing.T and fail.

@viambot viambot added the safe to test This pull request is marked safe to test from a trusted zone label Mar 15, 2024
@@ -42,10 +44,12 @@ type configAndCerts struct {
}

// NewFakeCloudServer creates and starts a new grpc server for the Viam Cloud.
func NewFakeCloudServer(ctx context.Context, logger logging.Logger) (*FakeCloudServer, error) {
func NewFakeCloudServer(t *testing.T, ctx context.Context, logger logging.Logger) (*FakeCloudServer, func()) { //revive:disable-line:context-as-argument
Copy link
Contributor Author

Choose a reason for hiding this comment

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

side note, I tried to configure this rule globally (allowing *testing.T to come before context.Context) in etc/.golangci.yaml but was unsuccessful. If others agree / know how to do this please share!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so I figured out how to configure this rule globally, but it has the side-effect of turning all other revive rules. furthermore, enable all rules explicitly seems to cause a panic due to lack of defaults 😞

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this turned into a rabbit hole... I think the problem i was describing goes away if we use a later version of revive, but there's another issue apparently: golangci/golangci-lint#4353

not worth the hassle right now, just sticking with a //nolint:revive

@maximpertsov maximpertsov marked this pull request as ready for review March 15, 2024 15:17
@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 Mar 15, 2024
@maximpertsov maximpertsov force-pushed the fake-server-interface branch from 1bb0195 to 36654a2 Compare March 15, 2024 16:35
@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 Mar 15, 2024
Copy link
Member

@dgottlieb dgottlieb left a comment

Choose a reason for hiding this comment

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

This is a great cleanup

@maximpertsov maximpertsov merged commit 3ec4395 into viamrobotics:main Mar 15, 2024
16 checks passed
@maximpertsov maximpertsov deleted the fake-server-interface branch March 15, 2024 17:39
biotinker pushed a commit to biotinker/robotcore that referenced this pull request Mar 18, 2024
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