[test_runner] Simplify use of channels in step interface #121

marcoguerri · 2020-06-16T07:09:21Z

NOTE: this is an API breaking change.

The interface of TestSteps so far has dictated that targets should be
forwarded to two different channels for success and failure. This makes
several parts of ConTest logic relatively complicated. In order to
simplify the framework, this braking change reduces the number out
output channels that a test step needs to handle to only one. The object
returned by the step might include error information for targets which
failed in the test step.

The logic of the test runner is not modular enough to accomodate the need for implementing a cleanup pipeline. This revision refactors the test runner so that the concept of pipeline can be re-used. The eventual goal is to deploy two pipelines, one for testing and one for cleanup.

This revision further simplifies the routing logic and removes the dependency of the pipeline over the list of targets. The latter is important to implement pipelines that might have a variable number of targets coming in that cannot be predicted beforehand.

After extracting the pipeline logic, the routing logic was still part of the pipeline and very monolithic. This patch extracts the routing logic into a dedicated structure and splits it into routeIn logic and routeOut logic, which makes it unit-testable.

Pipeline and routing logic are now split into different files. Unit tests for the routing logic have been added.

This patch adds support for implementing a cleanup pipeline, which runs cleanup steps, after the main test pipeline. One of the requirements of ConTest has always been that it should support a "cleanup" phase, and this adds support for it. The cleanup pipeline is described in the same was test steps have been described so far: and additional "cleanup" section in the test descriptor is deserialized to implement a pipeline which runs the required cleanup steps.

codecov · 2020-06-16T07:12:31Z

Codecov Report

Merging #121 into master will increase coverage by 1.44%.
The diff coverage is 72.19%.

@@            Coverage Diff             @@
##           master     #121      +/-   ##
==========================================
+ Coverage   63.14%   64.58%   +1.44%     
==========================================
  Files          51       58       +7     
  Lines        2846     3103     +257     
==========================================
+ Hits         1797     2004     +207     
- Misses        813      855      +42     
- Partials      236      244       +8

Flag	Coverage Δ
#integration	`63.41% <71.97%> (+1.55%)`	⬆️
#unittests	`20.21% <16.24%> (+3.99%)`	⬆️

Impacted Files	Coverage Δ
pkg/jobmanager/jobmanager.go	`66.95% <ø> (+6.14%)`	⬆️
pkg/jobmanager/status.go	`0.00% <0.00%> (ø)`
pkg/pluginregistry/errors.go	`0.00% <0.00%> (-100.00%)`	⬇️
pkg/target/target.go	`100.00% <ø> (ø)`
plugins/targetlocker/inmemory/inmemory.go	`66.32% <ø> (ø)`
plugins/targetlocker/noop/noop.go	`78.57% <ø> (ø)`
plugins/teststeps/echo/echo.go	`0.00% <0.00%> (ø)`
pkg/runner/job_runner.go	`48.12% <20.00%> (ø)`
pkg/job/job.go	`56.52% <23.07%> (-43.48%)`	⬇️
plugins/teststeps/teststeps.go	`38.23% <29.41%> (+7.28%)`	⬆️
... and 32 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cc4a4dc...47f9a1d. Read the comment docs.

marcoguerri · 2020-06-16T07:14:38Z

The relevant change to understand this PR is to be looked into the StepChannels struct, which changes from

type StepChannels struct {
    In  <-chan *target.Target
    Out chan<- *target.Target
    Err chan<- cerrors.TargetError
}

to

type StepChannels struct {
    In  <-chan *target.Target
    Out chan<- *target.Result
}

where target.Result is the following:

type Result struct {
        Target *Target
        Err    error
}

The interface of TestSteps so far has dictated that targets should be forwarded to two different channels for success and failure. This makes several parts of ConTest logic relatively complicated. In order to simplify the framework, this braking change reduces the number out output channels that a test step needs to handle to only one. The object returned by the step might include error information for targets which failed in the test step.

xaionaro

47f9a1d -- LGTM

xaionaro · 2020-07-30T13:38:44Z

pkg/runner/test_runner_pipeline.go

+					targetResultCh <- &target.Result{Target: t}
+				}
+			}()
+		}


just a thought:

It seems to me it is wrong when a pipeline instance knows it's position (mixed-up entities). Otherwise you require to understand more context while looking into this function. Though it does not worth time of inventing something else: never mind, anyway :)

xaionaro · 2020-07-30T13:46:48Z

pkg/runner/test_runner_route.go

+					log.Panicf("timeout while writing result for target %+v after %v", pendingTarget, r.timeouts.MessageTimeout)
+				}
+			}()
+
 		}


It would be nice to cover this function with unit-tests. I would like to try to propose a much simpler implementation of this function :)

xaionaro · 2020-07-30T13:53:40Z

pkg/runner/test_runner_route.go

+				select {
+				case <-terminate:
+					err = fmt.Errorf("termination requested for routing into %s", r.bundle.StepLabel)
+				case r.routingChannels.targetResult <- targetResult:
+				case <-time.After(r.timeouts.MessageTimeout):
+					log.Panicf("could not forward failed target (%+v) to the test runner: %v", t, targetResult.Err)
+				}


just a thought:

I see similar logic is duplicated a lot of times. And I see you tried to deduplicate it through writeTargets. And if I understand correctly you haven't reused the same method, because it attached to *TestRunner. However it consumes only timeouts, so: may be you might just move the function from *TestRunner to somewhere reachable from both entities?

xaionaro · 2020-07-30T13:59:05Z

pkg/runner/test_runner_route_test.go

-						stepInResult <- fmt.Errorf("not all targets received by teste step")
-					} else {
-						stepInResult <- nil
+						err = fmt.Errorf("not all targets received by teste step")


I think it's not the first time I see this typo: teste → test. Does "teste" mean something on Italian? :)

xaionaro · 2020-07-30T14:09:04Z

plugins/teststeps/example/example.go

-					if err := ev.Emit(testevent.Data{EventName: FailedEvent, Target: target, Payload: nil}); err != nil {
-						return fmt.Errorf("failed to emit failed event: %v", err)
+				} else {
+					log.Infof("target %v succeded", result.Target)


nitpicking:

A typo: succeded → succeeded

xaionaro · 2020-07-30T14:12:15Z

plugins/teststeps/example/example.go

+			case ch.Out <- result:
+				if result.Err != nil {


just a thought:

Since result is transmitter as a pointer, there a potential race condition here. We create a constraint to guarantee we will not change the result in the core. It seems to be not the best example.

Why not to emit the event before sending the result?

xaionaro · 2020-07-30T14:13:41Z

plugins/teststeps/example/example.go

+			}
+
+			select {
+			case <-cancel:


warning:

Missing passthrough or return nil.

xaionaro · 2020-07-30T14:15:42Z

plugins/teststeps/randecho/randecho.go

-				_ = ev.Emit(evData)
-				log.Infof("Run: target %s succeeded: %s", target, params.GetOne("text"))
-				ch.Out <- target
+				eventName = event.Name("TargetSucceeded")


nitpicking:

Extra event.Name(.

marcoguerri · 2020-12-21T06:54:18Z

Being superseded by #208

marcoguerri added 6 commits June 16, 2020 09:08

[test_runner] Split pipeline and router in different files

b37ced4

Pipeline and routing logic are now split into different files. Unit tests for the routing logic have been added.

[test_runner] Prepare support for cleanup steps

2db4e31

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jun 16, 2020

marcoguerri mentioned this pull request Jun 17, 2020

[test_runner] Simplify routing logic, reduce pipeline's dependencies #107

Merged

marcoguerri mentioned this pull request Jul 28, 2020

Remove AddField logic added in #107, leverage logrus logic #127

Closed

xaionaro approved these changes Jul 30, 2020

View reviewed changes

marcoguerri mentioned this pull request Sep 9, 2020

For each target tests #143

Merged

marcoguerri mentioned this pull request Oct 7, 2020

[RFC] Schema migration tool #148

Closed

marcoguerri mentioned this pull request Dec 17, 2020

Minor refactor stepRouter routeIin #207

Closed

marcoguerri closed this Dec 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[test_runner] Simplify use of channels in step interface #121

[test_runner] Simplify use of channels in step interface #121

marcoguerri commented Jun 16, 2020 •

edited

Loading

codecov bot commented Jun 16, 2020 •

edited

Loading

marcoguerri commented Jun 16, 2020

xaionaro left a comment •

edited

Loading

xaionaro Jul 30, 2020

xaionaro Jul 30, 2020

xaionaro Jul 30, 2020

xaionaro Jul 30, 2020

xaionaro Jul 30, 2020

xaionaro Jul 30, 2020

xaionaro Jul 30, 2020

xaionaro Jul 30, 2020

marcoguerri commented Dec 21, 2020

[test_runner] Simplify use of channels in step interface #121

[test_runner] Simplify use of channels in step interface #121

Conversation

marcoguerri commented Jun 16, 2020 • edited Loading

codecov bot commented Jun 16, 2020 • edited Loading

Codecov Report

marcoguerri commented Jun 16, 2020

xaionaro left a comment • 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

marcoguerri commented Dec 21, 2020

marcoguerri commented Jun 16, 2020 •

edited

Loading

codecov bot commented Jun 16, 2020 •

edited

Loading

xaionaro left a comment •

edited

Loading