[kubeops] Add tests for GetRelease #1434

latiif · 2020-01-10T15:26:59Z

Description of the change

Tests new functionality introduced by #1406 .

Benefits

Adds test for GetRelease in /pkg/agent
Adds action="get" tests for /kubeops/handler
Adds tests for dashboardcompat.go in /kubeops/internal/handler

Possible drawbacks

Skip tests blocked by memory driver bug

Applicable issues

Tests #1406

Additional information

Contains a TODO in /cmd/kubeops/internal/handler/handler_test.go with instructions on what to do when the memory driver bug is fixed.

Signed-off-by: Latiif abdullatif.alshriaf@combitech.se

andresmgot

Thanks for adding the tests @latiif! I have a few of small suggestions

andresmgot · 2020-01-10T16:02:40Z

cmd/kubeops/internal/handler/dashboardcompat_test.go

+					},
+				},
+			},
+			Helm2Release: h2.Release{


shouldn't this be empty if it fails?

Making the Helm2Release would make it fail, sure.
Here we test that all other information are identical, except for the version.

I see. The thing I was missing is that the function never fails (I doesn't return an error) so the ShouldFail param is a bit misleading. Actually this test is not testing any functional code. As far as I can see, you are giving a wrong expected result and verifying that the test is "failing" (so this is more a test for the test).

If I am correct with the above, I suggest to remove this test and remove the ShouldFail param for this to be more readable. Also, I would make it so all the Helm2Releases are equals to whatever we set in the test. That way, if both releases are not equal, we know exactly how it should behave in that case. I am talking about the next test: Incomplete Helm 3 Release

You are correct. I will remove it. Thanks for pointing it out.

andresmgot · 2020-01-10T16:07:13Z

cmd/kubeops/internal/handler/dashboardcompat_test.go

+	}
+}
+
+func asResponse(data h2.Release) string {


can you explain why you are using asResponse? Why not using directly a marshall function?

still not sure about this ^

In the current setting, the result from the function newDashboardCompatibleRelease is used in kubeops/internal/handler here & here, which both marshal the data using github.com/unrolled/render. To avoid digging in the aforementioned library and to enable further tests where newDashboardCompatibleRelease are marshalled/viewed in different ways, I created asResponse as a helper function to mimic the behaviour in /cmd/kubeops/internal/handler/handler.go.

If my reasoning seems too stretched I can just bake it into the test.

That's fine, a comment explaining that would be helpful for the future

andresmgot · 2020-01-10T16:08:06Z

cmd/kubeops/internal/handler/dashboardcompat_test.go

+		Description         string
+		Helm3Release        h3.Release
+		Helm2Release        h2.Release
+		MarshallingFunction func(h2.Release) string


this is always asResponse, in which case you plan to change this function?

If we add a test scenario where we test newDashboardCompatibleRelease against for example spew.Sdump, or a hand-written one where for example not all fields are relevant/populated etc.

andresmgot · 2020-01-10T16:10:14Z

cmd/kubeops/internal/handler/handler_test.go

+				// TODO Remove this `if` statement after the memory driver bug is fixed
+				if test.StatusCode == 404 && response.Code == 500 {
+					t.SkipNow()
+				}


this is a bit difficult to notice, I would rather add a field "skip: bool" to the test struct and explicitly mark some tests to be skipped (and add a link to the Github issue that should be fixed)

- Add test for GetRelease in /pkg/agent - Add 'action=get' tests for /kubeops/handler - Add tests for 'dashboardcompat.go' in /kubeops/handler - Replace newConfigFixture with newActionConfigFixture - Skip tests blocked by memory driver bug TODO: REMOVE CHANGES FROM THIS COMMIT WHEN MEMORY DRIVER IS FIXED Signed-off-by: Latiif <abdullatif.alshriaf@combitech.se>

- Remove non-functionality-testing tests. - Remove 'shouldFail' flag. - Capture panics and report them within the test suite. Signed-off-by: Latiif <abdullatif.alshriaf@combitech.se>

Signed-off-by: Latiif <abdullatif.alshriaf@combitech.se>

andresmgot

Thanks for the changes!

Signed-off-by: Latiif <abdullatif.alshriaf@combitech.se>

latiif · 2020-01-13T14:07:01Z

Comment added for:

The field MarshallingFunction
The function asResponse

For further improvements or tests on the dashboardcompat.go

absoludity · 2020-01-13T22:46:22Z

cmd/kubeops/internal/handler/dashboardcompat_test.go

+
+	for _, test := range tests {
+		t.Run(test.Description, func(t *testing.T) {
+			// Capture the panic and report it in an orderly fashion


I'm confused as to why we're handling a panic that we've discovered in tests, rather than removing the panic from the code? What causes the panic... if I run these tests without the panic handling, it just succeeds.

I came back to this later today, found the cause of the panic, added some failing tests and fixed. See what you think: #1445

andresmgot reviewed Jan 10, 2020

View reviewed changes

latiif force-pushed the test-helm3-get-release branch from 229ff47 to a070277 Compare January 11, 2020 09:43

latiif force-pushed the test-helm3-get-release branch from efd508f to 5d1511a Compare January 12, 2020 17:05

latiif mentioned this pull request Jan 12, 2020

Add tests for DeleteRelease #1438

Merged

latiif added 3 commits January 13, 2020 14:00

Merge branch 'master' into test-helm3-get-release

244112c

Improve tests for 'dashboardcompat.go'

a74e1b1

- Remove non-functionality-testing tests. - Remove 'shouldFail' flag. - Capture panics and report them within the test suite. Signed-off-by: Latiif <abdullatif.alshriaf@combitech.se>

Ensure order of releases from Memory Driver

2fabd14

Signed-off-by: Latiif <abdullatif.alshriaf@combitech.se>

andresmgot approved these changes Jan 13, 2020

View reviewed changes

Add comments to field "MarshallingFunction" and to function "asResponse"

eb54b98

Signed-off-by: Latiif <abdullatif.alshriaf@combitech.se>

andresmgot merged commit ac689ce into vmware-tanzu:master Jan 13, 2020

absoludity reviewed Jan 13, 2020

View reviewed changes

absoludity mentioned this pull request Jan 14, 2020

Test and remove panic possibilities in newDashboardCompatibleRelease #1445

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[kubeops] Add tests for GetRelease #1434

[kubeops] Add tests for GetRelease #1434

latiif commented Jan 10, 2020

andresmgot left a comment

andresmgot Jan 10, 2020

latiif Jan 11, 2020

andresmgot Jan 13, 2020 •

edited

Loading

latiif Jan 13, 2020

andresmgot Jan 10, 2020

andresmgot Jan 13, 2020

latiif Jan 13, 2020

andresmgot Jan 13, 2020

andresmgot Jan 10, 2020

latiif Jan 11, 2020

andresmgot Jan 10, 2020

andresmgot left a comment

latiif commented Jan 13, 2020

absoludity Jan 13, 2020

absoludity Jan 14, 2020

[kubeops] Add tests for GetRelease #1434

[kubeops] Add tests for GetRelease #1434

Conversation

latiif commented Jan 10, 2020

Description of the change

Benefits

Possible drawbacks

Applicable issues

Additional information

andresmgot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andresmgot Jan 13, 2020 • 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

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

andresmgot left a comment

Choose a reason for hiding this comment

latiif commented Jan 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andresmgot Jan 13, 2020 •

edited

Loading