[DRAFT] Add unit level tests for App repository #887

julian-hj · 2022-03-29T23:47:10Z

Is there a related GitHub Issue?

What is this change about?

We initially added units only to the App repo to establish the pattern. If folks feel this is a good way to go, we will make additional chores to fill out the other repos.

This allows us to reach better code coverage for error cases
We added interfaces for the namespace retriever and nsperms objects in
order to make them fakeable.
We added fakes for the client factory and client interfaces also.
Minor typo and format fixes.

Does this PR introduce a breaking change?

No

Acceptance Steps

It's a chore.

Tag your pair, your PM, and/or team

@matt-royal @gcapizzi @georgethebeatle @kieron-dev @danail-branekov

- This allows us to reach better code coverage for error cases - We added interfaces for the namespace retriever and nsperms objects in order to make them fakeable. - We added fakes for the client factory and client interfaces also. - Minor typo and format fixes. [#601] Co-authored-by: Julian Hjortshoj <hjortshojj@vmware.com>

kieron-dev

This generally looks good. As I put in a file comment, I think we should go for full coverage at this level and not skip the happy paths and the argument checking.

I'd also like to see what happens with the associated integration test. Is there duplication that can be removed?

kieron-dev · 2022-03-30T15:35:29Z

api/authorization/namespace_permissions.go

 	privilegedClient client.Client
 	identityProvider IdentityProvider
 	rootNamespace    string
 }

-func NewNamespacePermissions(privilegedClient client.Client, identityProvider IdentityProvider, rootNamespace string) *NamespacePermissions {
-	return &NamespacePermissions{
+func NewNamespacePermissions(privilegedClient client.Client, identityProvider IdentityProvider, rootNamespace string) *namespacePermissions {


Returning a private struct is interesting. Maybe we should do more of this.

I've been wanting to do this in the handlers as well. Having the type be private means that you can't instantiate it incorrectly outside of the constructor.

I have always liked this approach, but have avoided it for years, because it used to yield a golint linter warning. I tried to find out one more time why exactly returning an unexported type is "annoying" and found this issue. However I am not convinced that it is evil if you return an interface implementation. So I think I agree with you, just putting these references here for additional context.

I think I found a valid use case for the annoyance argument. Imagine the following code

package foo type thing struct {...} func NewThing() *thing{....}

You need to testdrive it, so you create a test package (foo_test):

package foo_test .... var myThing ???? // how do you var the thing to test so that you can use it in `BeforeEach`, etc?

Remember that the golang convention is that interfaces are defined wherever needed, not where they are implemented, therefore package foo might not have the Thing interface at all.

In this particular case, imagine you had to write unit tests for the namespacePermissions struct. How would you do that?

In the extreme case, you can create the interface in the test file itself, since that's where it's used. However, in our codebase I think it would be reasonable to use the expected interface type in the test, since that's how the code will actually be used in practice. In this case that would mean var nsPerm repositories.NamespacePermission. If that feels awkward since the interface is declared in a different package, then we could move the interfaces into a shared package that contains common interfaces used throughout the codebase.

Personally, I prefer this approach to having to worry about objects being constructed incorrectly.

Personally, I prefer this approach to having to worry about objects being constructed incorrectly.

Absolutely agree! We should be using constructors to create objects and make all objects' field private to guarantee that, there is no argument about it.

What I doubt is whether making structs private (i.e. make their names start with a lowercase letter) and returning pointers to those structs be annoying for testing. For example

func NewThing() *Thing

vs

func NewThing() *thing

you can create the interface in the test file itself
Feels like a workaround for the sake of testing to me. What about cases when a struct implements a couple of interfaces? Would you need the "productive" interfaces defined wherever they are needed and a test interface that combines them all?

then we could move the interfaces into a shared package that contains common interfaces used throughout the codebase.
I do not think that this is quite golang idiomatic though.

kieron-dev · 2022-03-30T15:38:51Z

api/repositories/app_repository_test.go

@@ -28,6 +31,263 @@ const (
 	CFAppStoppedState  = "STOPPED"
 )

+//counterfeiter:generate -o fake -fake-name ClientWithWatch sigs.k8s.io/controller-runtime/pkg/client.WithWatch
+var _ = Describe("AppRepository Units", func() {


I guess we'd split the integration tests out into a separate dir when this goes out of draft?

We were on the fence about that. We are currently in a strange state because we've been using integration level tests as unit tests, and measuring coverage against those tests. This PR backfills some testing to get to unreachable code, but doesn't attempt to get to full coverage on the unit test side.

Ideally, we would stop using integration tests as unit tests, and write unit tests that achieve full coverage on their own, and move the env tests into a separate suite, but that would take a lot of time, so in the interim we settled on putting both in the same file to make it easier to understand how they relate to one another to achieve test coverage.

So, yeah basically, I agree that the two kinds of tests should be separated, but I am less sure that immediately is the right time frame, unless we were going to tear off the bandage and rewrite all the tests.

That's something we wanted feedback on. In the past we've put unit and integration tests in separate packages. This makes it harder to find all of the tests and requires you to remember the structure of each different package. Having the unit tests in the same file with the integration tests solves this problem, but at the cost of slightly higher overhead per test (due to the BeforeEach targeted at test-env tests)

Even if we do not convert integrations to units right away I still think it is good to have a separation between those two. If they are separated we can agree on some convention of what goes where. For example we might agree that we only ever put error cases in the units. But if there are no integrations and units it is hard to even communicate that and I am afraid these suites will continue growing in a diffuse manner.

Another benefit of having separate suites is test execution speed as you have already mentioned.

I hear that some of the reasoning behind mixing the pure units with the test-env integrations might be that this way it is easier to see combined code coverage in the IDE. However not everyone uses the same IDE/editor, while we do have code climate coverage setup that is merging coverage from all tests no matter if they are in the same package or not (with the exception of e2es). To me this is a better place to look for uncovered branches as it is central and we can create chores with links to the reports.

kieron-dev · 2022-03-30T15:45:20Z

api/repositories/app_repository_test.go

+			_, retErr = appRepo.GetApp(testCtx, authInfo, "some-app-name")
+		})
+
+		When("the client factory reports an error", func() {


I would aim to cover as much as possible with the unit tests. So as well as covering the error paths, e.g. for GetApp, we should also:

check that the namespaceRetriever is invoked with the correct args

check the expected authInfo is passed to the userClientFactory

check that the expected namespace and name are requested in the userClient.Get

We saw a case recently where a couple of string arguments were passed in the wrong order, so as well as covering the untested error paths, these tests could guarantee calling out correctly.

I forgot to comment here that I pushed a change to add the arg checks you mentioned. I chose to skip the assertions against the userClient, though, since the test-env tests already verify that the correct data is being fetched. The test-env tests will also survive a refactor (e.g adding an indexed field to a resource), where the units will require changes.

matt-royal · 2022-03-30T18:07:20Z

@kieron-dev Thank you for the feedback on this PR. We added a discussion topic to the sync meeting tomorrow about this PR and the future work we'll spin out for other repos.

RE full unit test coverage: Personally, I would prefer to lean on the test-env integration tests for repositories where possible, and only fall back on the unit tests when we have a case that can't be exercised via integration. My rationale is that the test-env tests are easier to read and alter than the unit tests, and are a more realistic test of behavior. The downside is the occasional flake and somewhat slower tests, but for me this still balances out in the integration test's favor.

I agree with your comment about testing that the correct arguments are passed to the fakes in unit tests, and I'm going to modify this PR to add those.

[#601] Authored-by: Matt Royal <mroyal@vmware.com>

matt-royal · 2022-03-31T00:05:49Z

I pushed an update that adds the assertions you mentioned, @kieron-dev

julian-hj · 2022-04-05T16:08:50Z

Closing this PR, as discussed in the SIG meeting, due to an absence of proponents.

kieron-dev reviewed Mar 30, 2022

View reviewed changes

Integrate Kieron's feedback

85a87b8

[#601] Authored-by: Matt Royal <mroyal@vmware.com>

julian-hj closed this Apr 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DRAFT] Add unit level tests for App repository #887

[DRAFT] Add unit level tests for App repository #887

julian-hj commented Mar 29, 2022

kieron-dev left a comment

kieron-dev Mar 30, 2022

matt-royal Mar 30, 2022

georgethebeatle Mar 31, 2022 •

edited

Loading

danail-branekov Mar 31, 2022

matt-royal Mar 31, 2022 •

edited

Loading

danail-branekov Apr 1, 2022 •

edited

Loading

kieron-dev Mar 30, 2022

julian-hj Mar 30, 2022

matt-royal Mar 30, 2022

georgethebeatle Mar 31, 2022 •

edited

Loading

georgethebeatle Apr 4, 2022

kieron-dev Mar 30, 2022

matt-royal Apr 1, 2022

matt-royal commented Mar 30, 2022

matt-royal commented Mar 31, 2022

julian-hj commented Apr 5, 2022

[DRAFT] Add unit level tests for App repository #887

[DRAFT] Add unit level tests for App repository #887

Conversation

julian-hj commented Mar 29, 2022

Is there a related GitHub Issue?

What is this change about?

Does this PR introduce a breaking change?

Acceptance Steps

Tag your pair, your PM, and/or team

kieron-dev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

georgethebeatle Mar 31, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matt-royal Mar 31, 2022 • edited Loading

Choose a reason for hiding this comment

danail-branekov Apr 1, 2022 • edited Loading

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

georgethebeatle Mar 31, 2022 • edited Loading

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

matt-royal commented Mar 30, 2022

matt-royal commented Mar 31, 2022

julian-hj commented Apr 5, 2022

georgethebeatle Mar 31, 2022 •

edited

Loading

matt-royal Mar 31, 2022 •

edited

Loading

danail-branekov Apr 1, 2022 •

edited

Loading

georgethebeatle Mar 31, 2022 •

edited

Loading