Add to documentation how to run the unit tests

[winiciusallan]

Closes #563

[winiciusallan]

I thought that'd be good to have a new section about how to contribute to the docs, and describe how the contributor can set up the mkdocs.

If it makes sense, I can add to this PR or open another with the changes.

[mandre]

Thanks! This was indeed a gap in our developer documentation and I'm glad someone's taking the time to fix it. I've left a couple of comments, for possible improvement (it's a good start already).

I see we also talk about API validations at the top of the file, do we want to regroup that with unit tests?

We may also want to highlight these tests run in CI.

[mandre]

See also the test make target, which should probably be preferred over running go test directly, since it sets up envtest for controllers tests that have leverage it. You can filter which controller you want to run tests for with the TEST_PATHS environment variable.

openstack-resource-controller/Makefile

Lines 113 to 116 in 00c8ac6

	.PHONY: test
	TEST_PATHS ?= ./...
	test: envtest
	KUBEBUILDER_ASSETS="$(shell $(ENVTEST) use $(ENVTEST_K8S_VERSION) --bin-dir $(LOCALBIN) -p path)" go test $$(go list $(TEST_PATHS) | grep -v /e2e) -coverprofile cover.out

I believe image is currently the only controller using it.

[winiciusallan]

Yeah, I agree. Using target is good because it also runs tests that leverage envtest.

I can commit the change to include the make target and document the TEST_PATHS variable. What do you think?

[mandre]

That'd be great!

[mandre]

All controllers should have unit tests, however, because for the most part the controllers are boilerplate code, we've been a bit lax. We require unit tests for custom code, so if your controller handles mutability, there should be a test for each mutable field.

[winiciusallan]

Sorry, I think I didn't get this comment. Are you suggesting a replacement text or making a comment regarding the tests? In case of the first option, I can highlight those things that you've commented on; it makes a lot of sense.

[mandre]

Not necessarily, I wanted to highlight the reasons why we had an actuator_test.go for each controller that had tests for the update handlers. Perhaps it's still a good idea to be explicit about when we require tests (custom code, such as update handlers) in the docs. I'll leave it up to you.

[winiciusallan]

I see we also talk about API validations at the top of the file, do we want to regroup that with unit tests?

To be honest, I didn't see this section at the start of the page. Both API validation and actuator tests are unit tests in a sense, so it makes sense to have them under the same section, but it is important to separate them regarding their goals.

We may also want to highlight these tests run in CI.

+1

I think we can change the "Developing controllers" to include a step where the contributor may implement apivalidations tests, so we can have good coverage code for all of the controllers and check corner cases.

[mandre]

I see we also talk about API validations at the top of the file, do we want to regroup that with unit tests?

To be honest, I didn't see this section at the start of the page. Both API validation and actuator tests are unit tests in a sense, so it makes sense to have them under the same section, but it is important to separate them regarding their goals.

Yup, exactly.

We may also want to highlight these tests run in CI.

+1

I think we can change the "Developing controllers" to include a step where the contributor may implement apivalidations tests, so we can have good coverage code for all of the controllers and check corner cases.

We may also want to add a stub the API validation in the controller scaffolding tool to make it obvious we ideally want API validations unit tests, WDYT? That should be part of another PR if we decide it's a good idea.

[mandre]

I thought that'd be good to have a new section about how to contribute to the docs, and describe how the contributor can set up the mkdocs.

If it makes sense, I can add to this PR or open another with the changes.

If you feel it helps onboarding new contributors, I'm not going to block you, and would actually be very happy to merge any change that makes the docs better.

[winiciusallan]

We may also want to add a stub the API validation in the controller scaffolding tool to make it obvious we ideally want API validations unit tests, WDYT? That should be part of another PR if we decide it's a good idea.

It'd be great.

The scaffolding tool generates too many files, most of which are just boilerplate code, so newcomer contributors may feel a little lost. Could you please open an issue for this?

[winiciusallan]

I thought that'd be good to have a new section about how to contribute to the docs, and describe how the contributor can set up the mkdocs.
If it makes sense, I can add to this PR or open another with the changes.

If you feel it helps onboarding new contributors, I'm not going to block you, and would actually be very happy to merge any change that makes the docs better.

Do you think it will be necessary a new issue to address it?

[winiciusallan]

Sorry, I amended the commit and have lost discussions along the code.

Well, I made a few adjustments. Check if you think it is ready to go. Don't hesitate to ask for changes :)

[mandre]

It should be:

Suggested change

	$ TEST_PATHS=$(go list ./internal/controller/server ./internal/controller/image) make test
	$ TEST_PATHS="./internal/controllers/server ./internal/controllers/image" make test

[winiciusallan]

Done

[mandre]

Thoughts about moving lines 12-25 outside of the Mutability tests subsection since it applies to all unit tests?

[winiciusallan]

Looks good. Done!

[mandre]

Actually, the TEST_PATHS environment variable is given to go list to retrieve module names, so it must constain directory paths, and not module names.

[winiciusallan]

True. Thanks for the correction.