commands/.../scorecard: modularize tests and implement weights #994

AlexNPavel · 2019-01-24T23:15:38Z

Description of the change: This PR defines new structs for: the variables that scorecard tests need access to, scores, tests, and test suites. It defines each test and test suite using these new structs and now weights the tests based on how important they should be in the final score. It also splits checkSpecAndStat (as that was acutally 2 separate tests plus a readiness checker) and moves the readiness checking functionality into its own function.

Motivation for the change: Closes #960

commands/operator-sdk/cmd/scorecard/test_definitions.go

shawn-hurley · 2019-01-25T14:44:26Z

commands/operator-sdk/cmd/scorecard/test_definitions.go

+	retryInterval int
+}
+
+type Score struct {


AlexNPavel · 2019-02-07T00:33:35Z

This PR has been refactored with the new design suggestions

estroz

Overall looks great, but I'd like to see unification of return types and method receivers as per my comment.

commands/operator-sdk/cmd/scorecard/scorecard.go

commands/operator-sdk/cmd/scorecard/resource_handler.go

commands/operator-sdk/cmd/scorecard/test_definitions.go

commands/operator-sdk/cmd/scorecard/basic_tests.go

hasbro17 · 2019-03-04T06:53:11Z

Overall SGTM. Just some nits and perhaps we can add more godocs for all the new structs in test_definitions.go.

Co-Authored-By: AlexNPavel <[email protected]>

AlexNPavel · 2019-03-04T21:43:43Z

Whoops, hit the wrong button

AlexNPavel · 2019-03-04T21:43:55Z

/cc @shawn-hurley @joelanford

commands/operator-sdk/cmd/scorecard/scorecard.go

hasbro17

LGTM after nit

hasbro17 · 2019-03-04T23:01:43Z

Unrelated to the scope of this PR but just wanted to make a note on SpecDescriptorsTest and StatusDescriptorsTest that we could address later on:
Not all spec and status fields need to have a spec and status descriptor.
It's only recommended to have descriptors for spec and status fields that are useful to show on the console.
See the descriptors section in https://github.com/operator-framework/operator-lifecycle-manager/blob/master/Documentation/design/building-your-csv.md#owned-crds

So it's likely that no CSVs will ever fully pass those tests. Perhaps we should make note of that in the Suggestions for these tests to not force users to assign descriptors to all fields.
@estroz Might be related to #1162 Have to take a look at that but I'm not sure if we generate descriptors for all spec and status fields.

Co-Authored-By: AlexNPavel <[email protected]>

joelanford

LGTM

I did notice that we're using global variables in a few places (e.g. deploymentName, proxyPodGlobal), and I'm wondering if it's possible to eliminate more of them. I don't think that's necessary in this PR, but something to think about for a future PR.

shawn-hurley · 2019-03-05T21:59:50Z

commands/operator-sdk/cmd/scorecard/basic_tests.go

-	}
-	if err != nil && err != wait.ErrWaitTimeout {
-		return err
+// checkSpec checks that the spec block exists


I don't think this godoc is correct.

shawn-hurley · 2019-03-05T22:00:30Z

commands/operator-sdk/cmd/scorecard/basic_tests.go

-func checkStatusUpdate(runtimeClient client.Client, obj *unstructured.Unstructured) error {
-	test := scorecardTest{testType: basicOperator, name: "Operator actions are reflected in status", maximumPoints: 1}
-	err := runtimeClient.Get(context.TODO(), types.NamespacedName{Namespace: obj.GetNamespace(), Name: obj.GetName()}, obj)
+// checkStatus checks that the status block exists


I don't think this godoc is correct

shawn-hurley · 2019-03-05T22:03:23Z

commands/operator-sdk/cmd/scorecard/test_definitions.go

+	}
+}
+
+type WritingIntoCRsHasEffectTest struct {


Godoc for these types please

shawn-hurley

LGTM except for godocs and potential layout changes

shawn-hurley · 2019-03-05T22:03:37Z

commands/operator-sdk/cmd/scorecard/test_definitions.go

+	BasicTestConfig
+}
+
+func NewWritingIntoCRsHasEffectTest(conf BasicTestConfig) *WritingIntoCRsHasEffectTest {


godoc for these functions please

shawn-hurley · 2019-03-05T22:04:31Z

commands/operator-sdk/cmd/scorecard/test_definitions.go

+	}
+}
+
+type WritingIntoCRsHasEffectTest struct {


Have we considered making files for each type with its own run function next to it?

It is a minor layout thing, but will make it easier to find the actual test later one IMO.

That would be a useful change and make it easier to find things. I think that should be done in a follow up PR though.

AlexNPavel · 2019-03-06T19:36:31Z

Added godocs for all new exported functions, structs, and interface.

/cc @shawn-hurley

commands/.../scorecard: modularize tests and implement weights

11b1270

AlexNPavel requested review from mhrivnak, joelanford, shawn-hurley, lilic, hasbro17 and estroz January 24, 2019 23:15

openshift-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jan 24, 2019

shawn-hurley requested changes Jan 25, 2019

View reviewed changes

commands/.../scorecard/test_definitions: add some godoc

c786e4f

AlexNPavel force-pushed the modularize-scorecard branch from 3f2bb45 to c786e4f Compare January 25, 2019 20:09

AlexNPavel added 3 commits January 25, 2019 12:12

Merge branch 'master' into modularize-scorecard

926b8ee

Merge branch 'master' into modularize-scorecard

550cb3e

Merge branch 'master' into modularize-scorecard

2af721a

AlexNPavel force-pushed the modularize-scorecard branch from 7e3ea09 to 2af721a Compare January 31, 2019 22:40

Merge branch 'master' into modularize-scorecard

86d7eda

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 6, 2019

AlexNPavel added 2 commits February 6, 2019 15:47

*: refactor

4858bd8

Merge branch 'master' into modularize-scorecard

ee40142

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 6, 2019

AlexNPavel added 4 commits February 6, 2019 16:16

commands/.../scorecard: fix unit test

cdf83af

commands/.../scorecard: correct error logging

cf4c1ff

commands/.../scorecard: correctly pass ctx in ts.Run

e51dfa3

commands/.../scorecard: move some functions

33984a8

AlexNPavel force-pushed the modularize-scorecard branch from a26f1e1 to 33984a8 Compare February 7, 2019 00:31

estroz suggested changes Feb 7, 2019

View reviewed changes

commands/operator-sdk/cmd/scorecard/scorecard.go Outdated Show resolved Hide resolved

commands/.../scorecard: use pointers to structs

aeeda47

estroz approved these changes Feb 7, 2019

View reviewed changes

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 9, 2019

hasbro17 reviewed Mar 4, 2019

View reviewed changes

commands/operator-sdk/cmd/scorecard/resource_handler.go Outdated Show resolved Hide resolved

hasbro17 reviewed Mar 4, 2019

View reviewed changes

commands/operator-sdk/cmd/scorecard/test_definitions.go Outdated Show resolved Hide resolved

hasbro17 reviewed Mar 4, 2019

View reviewed changes

commands/operator-sdk/cmd/scorecard/basic_tests.go Outdated Show resolved Hide resolved

hasbro17 reviewed Mar 4, 2019

View reviewed changes

commands/operator-sdk/cmd/scorecard/basic_tests.go Show resolved Hide resolved

hasbro17 and others added 4 commits March 4, 2019 13:09

Apply suggestions from code review

edbcbec

Co-Authored-By: AlexNPavel <[email protected]>

commands/.../scorecard: rename waitUntilReady

a531e27

commands/.../scorecard: add comment about cumulative scores

633f8dd

commands/.../scorecard: add a missing Run godoc

d098f05

AlexNPavel closed this Mar 4, 2019

AlexNPavel reopened this Mar 4, 2019

openshift-ci-robot requested review from joelanford and shawn-hurley March 4, 2019 21:43

hasbro17 reviewed Mar 4, 2019

View reviewed changes

commands/operator-sdk/cmd/scorecard/scorecard.go Outdated Show resolved Hide resolved

hasbro17 approved these changes Mar 4, 2019

View reviewed changes

Update commands/operator-sdk/cmd/scorecard/scorecard.go

f328e85

Co-Authored-By: AlexNPavel <[email protected]>

joelanford approved these changes Mar 5, 2019

View reviewed changes

shawn-hurley reviewed Mar 5, 2019

View reviewed changes

shawn-hurley requested changes Mar 5, 2019

View reviewed changes

commands/.../scorecard: add more godocs

191dd1b

openshift-ci-robot requested a review from shawn-hurley March 6, 2019 19:36

shawn-hurley approved these changes Mar 6, 2019

View reviewed changes

AlexNPavel merged commit e32aada into operator-framework:master Mar 6, 2019

AlexNPavel deleted the modularize-scorecard branch March 6, 2019 21:29

commands/.../scorecard: modularize tests and implement weights #994

commands/.../scorecard: modularize tests and implement weights #994

Uh oh!

Conversation

AlexNPavel commented Jan 24, 2019

Uh oh!

Uh oh!

shawn-hurley Jan 25, 2019

Choose a reason for hiding this comment

Uh oh!

AlexNPavel commented Feb 7, 2019

Uh oh!

estroz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hasbro17 commented Mar 4, 2019

Uh oh!

AlexNPavel commented Mar 4, 2019

Uh oh!

AlexNPavel commented Mar 4, 2019

Uh oh!

Uh oh!

hasbro17 left a comment

Choose a reason for hiding this comment

Uh oh!

hasbro17 commented Mar 4, 2019

Uh oh!

joelanford left a comment

Choose a reason for hiding this comment

Uh oh!

shawn-hurley Mar 5, 2019

Choose a reason for hiding this comment

Uh oh!

shawn-hurley Mar 5, 2019

Choose a reason for hiding this comment

Uh oh!

shawn-hurley Mar 5, 2019

Choose a reason for hiding this comment

Uh oh!

shawn-hurley left a comment

Choose a reason for hiding this comment

Uh oh!

shawn-hurley Mar 5, 2019

Choose a reason for hiding this comment

Uh oh!

shawn-hurley Mar 5, 2019

Choose a reason for hiding this comment

Uh oh!

AlexNPavel Mar 6, 2019

Choose a reason for hiding this comment

Uh oh!

AlexNPavel commented Mar 6, 2019

Uh oh!

Uh oh!