ci-operator multi-stage: MultiStageTestStep #100

bbguimaraes · 2019-08-26T19:20:04Z

Basic implementation of the multi-stage test type. Subset of
#85, based on
#98. Design document:
https://docs.google.com/document/d/1md-1BMf4_7mtKgGVoeZ3jOh4zSIBSjwl6vTTAYESwIM.

Fully-functional but lacking management of artifacts, the test secret, cluster
profile, and release image(s).

petr-muller

Pretty well readable ever for me not really knowing what to expect here, nice job!

Some comments and questions inline.

petr-muller · 2019-08-27T09:32:31Z

pkg/api/types_test.go

+		RpmBuildCommands:        "make rpms",
+		Images:                  []ProjectDirectoryImageBuildStepConfiguration{{To: "img"}},
+	}
+	for _, i := range []string{


Personally I would prefer this to be t.Run() based so it's executed in parallel and does not stop on first failure.

petr-muller · 2019-08-27T09:35:05Z

pkg/defaults/defaults.go

+			if testStep.MultiStageTestConfiguration != nil {
+				var err error
+				if step, err = steps.MultiStageTestStep(*testStep, config, podClient, jobSpec); err != nil {
+					return nil, nil, fmt.Errorf("unable to create end to end test step: %v", err)


Are we actually calling this end to end step in the user-facing documentation? The design doc calls it multi-stage tests, I think we should call it that way even here.

No longer relevant because of #100 (comment).

pkg/steps/multi_stage.go

petr-muller · 2019-08-27T09:47:40Z

pkg/steps/multi_stage.go

+func (s *multiStageTestStep) Done() (bool, error) { return false, nil }
+func (s *multiStageTestStep) Name() string        { return s.name }
+func (s *multiStageTestStep) Description() string {
+	return fmt.Sprintf("Run test %s", s.name)


"Run multi-stage test..."?

pkg/steps/multi_stage.go

petr-muller · 2019-08-27T09:55:15Z

pkg/steps/multi_stage.go

+}
+func (s *multiStageTestStep) SubTests() []*junit.TestCase { return s.subTests }
+
+func (s multiStageTestStep) runSteps(ctx context.Context, steps []api.TestStep, shortCircuit bool) error {


Hmm... is the non-pointer receiver here intentional?

Yes. Is there a problem?

Well, that's what I asked.

Using a value receiver was based on the (reasonable but not optimal) assumption that it is what methods that don't mutate the object should use. After reading some golang nonsense, I suppose the size of the object is the contention here? Changed.

If we don't mutate the object, we don't need to pass the pointer.

Right but we also don't need to copy the values around every time either
/shrug

petr-muller · 2019-08-27T10:00:04Z

pkg/steps/multi_stage.go

+	if errs != nil {
+		return nil, utilerrors.NewAggregate(errs)
+	}
+	return ret, nil


This could be simplified to just return ret, utilerrors.NewAggregate(errs) as NewAggregate is documented to return nil for an empty slice.

Didn't know that.

petr-muller · 2019-08-27T10:00:56Z

pkg/steps/multi_stage.go

+	for _, step := range steps {
+		image := step.Image
+		if s.config.IsPipelineImage(step.Image) {
+			image = api.PipelineImageStream + ":" + step.Image


would prefer Sprintf over concatenation

petr-muller · 2019-08-27T10:01:27Z

pkg/steps/multi_stage.go

+			errs = append(errs, err)
+			continue
+		}
+		name := s.name + "-" + step.Name


pkg/steps/multi_stage.go

droslean · 2019-08-27T12:48:44Z

pkg/steps/multi_stage.go

+	return s.runPods(ctx, pods, shortCircuit)
+}
+
+func (s multiStageTestStep) genPods(steps []api.TestStep) ([]coreapi.Pod, error) {


No need to cut the name here. let it be generatePods

droslean · 2019-08-27T12:51:12Z

pkg/steps/multi_stage.go

+				break
+			}
+		}
+		if err := waitForPodCompletion(s.podClient.Pods(s.jobSpec.Namespace), pod.Name, nil, false); err != nil {


Do we need to pass a notifier here?

Is this outstanding?

droslean · 2019-08-27T12:51:50Z

pkg/steps/multi_stage.go

+		}
+	}
+	if errs == nil {
+		return nil


no need for that

droslean · 2019-08-27T13:02:05Z

pkg/steps/multi_stage.go

+	podClient PodClient,
+	jobSpec *api.JobSpec,
+) (api.Step, error) {
+	return newMultiStageTestStep(testConfig, config, podClient, jobSpec), nil


why we need an extra unexported function and pass again the same args there, to return a new struct?

Also, this doesn't return any error.

Error removed, the extra function is used by the tests.

pkg/steps/multi_stage.go

stevekuznetsov · 2019-08-27T16:26:15Z

pkg/api/types_test.go

+		want: true,
+	}} {
+		t.Run(tc.name, func(t *testing.T) {
+			if ret := tc.conf.BuildsImage("image"); ret != tc.want {


"image" here is a critical input to understanding a test case -- let's add it as a field to the test struct

Not sure what you mean, do you want image: "image" repeated in all cases?

I read

{ name: "not in `images`", conf: ReleaseBuildConfiguration{ Images: []ProjectDirectoryImageBuildStepConfiguration{ {To: "egami"}, }, }, }

As a test case and it's not obvious to me what is or is not in the list, and even what we expect the output to be

I think it's easier to read the test code here (a one-line conditional), but… changed.

stevekuznetsov

Looking great. Two things I'd like to see:

a test that starts from CI Operator configuration YAML and tests the output YAML for the Pods end-to-end, with all of the bells and whistles -- artifacts, resources, etc
where are we calling resolver.Resolve? before we do step = steps.MultiStageTestStep(*testStep, config, podClient, jobSpec) right?

pkg/api/config.go

pkg/api/types.go

stevekuznetsov · 2019-08-27T19:58:47Z

pkg/api/types_test.go

+		want: true,
+	}} {
+		t.Run(tc.name, func(t *testing.T) {
+			if ret := tc.conf.BuildsImage("image"); ret != tc.want {


stevekuznetsov · 2019-08-27T19:59:25Z

pkg/api/types_test.go

+		want: true,
+	}} {
+		t.Run(tc.name, func(t *testing.T) {
+			if ret := tc.conf.BuildsImage("image"); ret != tc.want {


I read

{ name: "not in `images`", conf: ReleaseBuildConfiguration{ Images: []ProjectDirectoryImageBuildStepConfiguration{ {To: "egami"}, }, }, }

As a test case and it's not obvious to me what is or is not in the list, and even what we expect the output to be

stevekuznetsov · 2019-08-27T20:05:50Z

pkg/steps/multi_stage.go

+	podClient       PodClient
+	jobSpec         *api.JobSpec
+	pre, test, post []api.TestStep
+	subTests        []*junit.TestCase


We should elide this until we do the follow-up to add it

stevekuznetsov · 2019-08-27T20:19:48Z

pkg/steps/multi_stage_test.go

+	}
+}
+
+func TestGenPods(t *testing.T) {


nit: TestGeneratePods

stevekuznetsov · 2019-08-27T20:23:26Z

pkg/steps/multi_stage_test.go

+	expected := []coreapi.Pod{{
+		ObjectMeta: meta.ObjectMeta{
+			Name: "test-step0",
+			Labels: map[string]string{


Shouldn't we be expecting org/repo/branch here?

This was based on master before #92 merged.

So shouldn't this test be failing right now?

I should have been more explicit in my comment, I corrected that when I rebased.

pkg/steps/multi_stage_test.go

stevekuznetsov · 2019-08-27T20:25:40Z

pkg/steps/multi_stage_test.go

+			var pods []*coreapi.Pod
+			fakecs.PrependReactor("create", "pods", func(action clientgo_testing.Action) (bool, runtime.Object, error) {
+				pod := action.(clientgo_testing.CreateAction).GetObject().(*coreapi.Pod)
+				for _, f := range tc.failures {


nit: we're not running out of bytes in GitHub's git servers so let's use full variable names

You'll hear from my orthopedist.

stevekuznetsov · 2019-08-27T20:27:21Z

pkg/steps/multi_stage_test.go

+				names = append(names, p.ObjectMeta.Name)
+			}
+			if !reflect.DeepEqual(names, tc.expected) {
+				t.Error(diff.ObjectReflectDiff(names, tc.expected))


In general for people who have never touched the tests in this file, it would be nice to be much more verbose:

t.Errorf("%s: did not execute correct pods: %v", testCase.name, diff.ObjectReflectDiff(names, tc.expected))

In addition, I find it nice to explain what and why in the test name, like "test step fails so no other test steps run" or "pre step fails so no test steps run"

bbguimaraes · 2019-08-28T07:52:27Z

Rebased to include #92, but only 185ce21^..1e7f9a4 are new commits.

stevekuznetsov

Couple things seem outstanding otherwise this LGTM

stevekuznetsov · 2019-08-28T22:10:50Z

pkg/steps/multi_stage.go

+				break
+			}
+		}
+		if err := waitForPodCompletion(s.podClient.Pods(s.jobSpec.Namespace), pod.Name, nil, false); err != nil {


Is this outstanding?

stevekuznetsov · 2019-08-28T22:11:04Z

pkg/steps/multi_stage.go

+			}.AsSelector().String(),
+		},
+	)
+	if errors.IsNotFound(err) {


This seems outstanding?

pkg/steps/multi_stage_test.go

bbguimaraes · 2019-08-29T16:30:28Z

I think all reviews have been addressed. Rebased, squashed.

bbguimaraes · 2019-08-29T16:35:03Z

Follow-up work identified in the review (other than those listed in the original description):

add tests for the validation code changed in this PR
log pods as JSON in dry-run mode

stevekuznetsov

Let's either resolve the comment about the notifier if it's not needed or add it as a sub-task to the JIRA card so we don't lose it.

/lgtm
/approve
/hold

Whenever you do whatever is appropriate there, @bbguimaraes feel free to cancel the hold

openshift-ci-robot · 2019-08-29T17:16:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bbguimaraes, stevekuznetsov

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [bbguimaraes,stevekuznetsov]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

bbguimaraes · 2019-08-29T17:27:12Z

Ah, it took me a while to understand what the notifier question was about. The final version of the code, with artifact handling, does pass an artifact worker as a notifier — that's what I was thinking about when I read and responded to the question.

It is not necessary here because the lifetime of the pod is exactly how much we want to wait. There is even a higher-level function we could use that does exactly that, but it would add noise to the subsequent PRs.

stevekuznetsov · 2019-08-29T17:34:58Z

ACK

/hold cancel

…artPod This seems generically useful, and not something that needs to be restricted to multi-step, where it has been since 8cf5f87 (ci-operator multi-stage: MultiStageTestStep, 2019-07-30, openshift#100).

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

openshift-ci-robot requested review from droslean and petr-muller August 26, 2019 19:20

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 26, 2019

bbguimaraes force-pushed the steps0 branch from 227147b to 30cddf0 Compare August 26, 2019 19:21

petr-muller reviewed Aug 27, 2019

View reviewed changes

droslean reviewed Aug 27, 2019

View reviewed changes

pkg/steps/multi_stage.go Outdated

}

}

if errs == nil {

return nil

Copy link

Member

droslean Aug 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need for that

droslean reviewed Aug 27, 2019

View reviewed changes

pkg/steps/multi_stage.go Show resolved Hide resolved

droslean reviewed Aug 27, 2019

View reviewed changes

pkg/steps/multi_stage.go Show resolved Hide resolved

stevekuznetsov reviewed Aug 27, 2019

View reviewed changes

bbguimaraes force-pushed the steps0 branch from 30cddf0 to 35aa748 Compare August 27, 2019 16:43

stevekuznetsov reviewed Aug 27, 2019

View reviewed changes

bbguimaraes force-pushed the steps0 branch from 35aa748 to 1e7f9a4 Compare August 28, 2019 07:44

openshift-ci-robot added the ¯\_(ツ)_/¯ ¯\\\_(ツ)_/¯ label Aug 28, 2019

stevekuznetsov reviewed Aug 28, 2019

View reviewed changes

bbguimaraes added 2 commits August 29, 2019 16:27

ci-operator: add helpers to determine image origin

ade36af

ci-operator multi-stage: MultiStageTestStep

8cf5f87

bbguimaraes force-pushed the steps0 branch from 246a2bd to 8cf5f87 Compare August 29, 2019 16:28

stevekuznetsov reviewed Aug 29, 2019

View reviewed changes

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 29, 2019

openshift-ci-robot assigned stevekuznetsov Aug 29, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 29, 2019

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 29, 2019

openshift-merge-robot merged commit ffe7773 into openshift:master Aug 29, 2019

bbguimaraes deleted the steps0 branch August 30, 2019 07:47

wking mentioned this pull request Oct 8, 2020

pkg/steps/template: Push "Executing pod..." logging into createOrRestartPod #1284

Merged

ci-operator multi-stage: MultiStageTestStep #100

ci-operator multi-stage: MultiStageTestStep #100

Uh oh!

Conversation

bbguimaraes commented Aug 26, 2019

Uh oh!

petr-muller left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stevekuznetsov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment