RSDK-6925 better helpers for resource manager tests #3795

maximpertsov · 2024-04-10T21:34:07Z

Adds a test helper version of robotimpl.New, which creates a local robot and automatically closes itself, and also fails tests on any failure. This fairly small helper ends up eliminating a lot of boilerplate code.

In addition, this PR also converts defer calls in test helpers to use T.Cleanup instead.

Depends on: Allow *Testing.T argument before context #3800

maximpertsov · 2024-04-10T21:37:30Z

robot/impl/utils.go

This file is a duplicate of robot/impltest/local_robot.go for tests that are in the robotimpl package. The TODO explains why we need both. If anyone has an idea of how to avoid this without making major restructures I'm all ears.

this seems like a nice compromise

If you can't create a testing utilities package (due to docs, dependencies, etc.), try to move testing utiities to a single go file, and add comment //go:build testing on the top of the file. Then you can run tests with go test -tags testing ./.... This codes won't be contained in go build binaries.

this has it's own problems: my linter complains unless this build tag is present on each test that uses this helper function - I imagine this will happen for others. And sadly there is no builtin testing build tag that is exposed to users. Proposals to add this have been declined thus far:

proposal: add a build tag "test" golang/go#21360

Proposal: automatically set a "test" tag during "go test" golang/go#14668

yeah this breaks linting in other ways... okay I'm not going to down this path. will just have two duplicate helper functions for now

maximpertsov · 2024-04-11T16:28:23Z

robot/impl/local_robot_test.go

 				remoteBot, ok := r3.RemoteByName("foo")
 				test.That(t, ok, test.ShouldBeFalse)
 				test.That(t, remoteBot, test.ShouldBeNil)

 				remoteConfig.Remotes[0].Auth.Entity = entityName
 				remoteConfig.Remotes[1].Auth.Entity = entityName
-				_, shutdown = initTestRobot(t, context.Background(), remoteConfig, logger)
-				shutdown()


we have some tests like this where we initialize a local robot and immediately close it. the new version of this test line captures the spirit of the assertion more explicitly IMO, since it's not obvious what shutdown does without visiting the helper.

maximpertsov · 2024-04-11T16:30:50Z

robot/impl/local_robot_test.go

-//revive:disable-next-line:context-as-argument
-func initTestRobot(t *testing.T, ctx context.Context, cfg *config.Config, logger logging.Logger) (robot.LocalRobot, func()) {
-	t.Helper()
-
-	r, err := robotimpl.New(ctx, cfg, logger)
-	test.That(t, r, test.ShouldNotBeNil)
-	test.That(t, err, test.ShouldBeNil)
-	return r, func() {
-		test.That(t, r.Close(ctx), test.ShouldBeNil)
-	}
-}


just a note that this helper served as the model the share helper I am adding in this PR - the only real difference is the use of t.Cleanup instead of returning a cleanup function

benjirewis

Nice; I generally like the idea of relying more on t.Cleanup. Have a question about double closing.

Using the Cleanup pattern means that the "cleanup" of the test could be in the actual test or could be in some registered Cleanup function from a helper. I don't love that ambiguity, but as long as we're consistently moving toward Cleanup, I think it's probably a cleaner pattern than what we have now. We have a number of test failures in the NetCode "todo" that seem to have to do with forgetting to Close some resource at the end of a test.

robot/impltest/local_robot.go

benjirewis · 2024-04-11T18:56:28Z

robot/impltest/local_robot.go

+// test if it cannot. It automatically closes itself after the test and all subtests
+// complete.
+//
+//nolint:revive


What is revive complaining about for these helpers?

context not being first argument - I tried to change this in our lint configurations but ended up having a very bad time, so just gonna have this ugly disable lint line. I'll add a comment so folks know why it's here

here's more context on my bad time

i'm going to try to have a better time - I opened a PR to configure this more broadly

merged - we don't need this lint exception anymore

benjirewis · 2024-04-11T18:57:51Z

robot/impl/resource_manager_modular_test.go

 		actualR := r.(*localRobot)
 		actualR.manager.moduleManager = mod

 		resource.RegisterAPI(compAPI,
 			resource.APIRegistration[resource.Resource]{ReflectRPCServiceDesc: &desc.ServiceDescriptor{}})
+		t.Cleanup(func() {


I assume multiple Cleanup functions use a stack in the same way as defer?

yes per docs

Cleanup functions will be called in last added, first called order.

cheukt

general idea looks great, one question

cheukt · 2024-04-12T19:20:46Z

robot/impl/utils.go

+	"go.viam.com/rdk/robot"
+)
+
+// TODO: this duplicates `robotimpltest.LocalRobot` for tests that are in the `robotimpl`


whats the TODO here?

removing the duplicate helper, if possible - here's some context on how I tried (and failed) to do that: #3795 (comment)

I will add a ticket we can link this TODO to

benjirewis

LGTM! Good ideas 🧑‍🔧 .

maximpertsov · 2024-04-12T20:19:59Z

LGTM! Good ideas 🧑‍🔧 .

All stolen from folks who know better than me 👼

viambot added the safe to test This pull request is marked safe to test from a trusted zone label Apr 10, 2024

maximpertsov commented Apr 10, 2024

View reviewed changes

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 Apr 10, 2024

maximpertsov force-pushed the RSDK-6925-resource-manager-improve-tests branch from 437156f to 6cf2adc Compare April 10, 2024 22:41

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 Apr 10, 2024

maximpertsov marked this pull request as ready for review April 11, 2024 16:00

maximpertsov requested review from benjirewis and cheukt April 11, 2024 16:01

maximpertsov commented Apr 11, 2024

View reviewed changes

benjirewis reviewed Apr 11, 2024

View reviewed changes

maximpertsov mentioned this pull request Apr 12, 2024

Allow *Testing.T argument before context #3800

Merged

maximpertsov added 12 commits April 12, 2024 12:11

close manager in helper

3de9053

setup local robot helper

9965e81

better cleanup in modular test helper

ad4a4f7

cleanup local robot test

920524c

use setup local robot in reconfigure tests

3084439

use setup local robot in discovery tests

8fb884c

robotimpltest package

dac3d3d

wip: centralize setLocalRobot as duplicate

8e6c369

TODO

724d61d

lint

599cf3f

remove more unnecessary defer closes

f6fc104

revive revive lints

352a485

maximpertsov force-pushed the RSDK-6925-resource-manager-improve-tests branch from 719956d to 352a485 Compare April 12, 2024 16:12

viambot removed the safe to test This pull request is marked safe to test from a trusted zone label Apr 12, 2024

viambot added the safe to test This pull request is marked safe to test from a trusted zone label Apr 12, 2024

maximpertsov requested a review from benjirewis April 12, 2024 16:13

cheukt reviewed Apr 12, 2024

View reviewed changes

link TODO to ticket

f65c198

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 Apr 12, 2024

maximpertsov requested a review from cheukt April 12, 2024 19:37

benjirewis approved these changes Apr 12, 2024

View reviewed changes

maximpertsov merged commit 4be1097 into viamrobotics:main Apr 12, 2024
16 checks passed

maximpertsov deleted the RSDK-6925-resource-manager-improve-tests branch April 12, 2024 21:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RSDK-6925 better helpers for resource manager tests #3795

RSDK-6925 better helpers for resource manager tests #3795

maximpertsov commented Apr 10, 2024 •

edited

Loading

maximpertsov Apr 10, 2024

maximpertsov Apr 11, 2024 •

edited

Loading

maximpertsov Apr 11, 2024

maximpertsov Apr 11, 2024 •

edited

Loading

maximpertsov Apr 11, 2024

maximpertsov Apr 11, 2024

benjirewis left a comment

benjirewis Apr 11, 2024

maximpertsov Apr 11, 2024

maximpertsov Apr 11, 2024

maximpertsov Apr 11, 2024

maximpertsov Apr 12, 2024

benjirewis Apr 11, 2024

maximpertsov Apr 11, 2024

cheukt left a comment

cheukt Apr 12, 2024

maximpertsov Apr 12, 2024

maximpertsov Apr 12, 2024

benjirewis left a comment

maximpertsov commented Apr 12, 2024

RSDK-6925 better helpers for resource manager tests #3795

RSDK-6925 better helpers for resource manager tests #3795

Conversation

maximpertsov commented Apr 10, 2024 • edited Loading

Choose a reason for hiding this comment

maximpertsov Apr 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maximpertsov Apr 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjirewis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cheukt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjirewis left a comment

Choose a reason for hiding this comment

maximpertsov commented Apr 12, 2024

maximpertsov commented Apr 10, 2024 •

edited

Loading

maximpertsov Apr 11, 2024 •

edited

Loading

maximpertsov Apr 11, 2024 •

edited

Loading