Improve testability of MultiSteps Scenario Runner

[BartRobeyns]

This technical debt issue is a result of a discussion in #263, recognizing the need for testability of the MultiStepsScenario Runner.

To get there, responsibilities should be split out into different classes/methods, and participant classes should become mockable/verifiable.

Objective

Ideally, after refactoring this method, we would be able to unit-test individual responsibilities like:

• when does the retry-mechanism stop, how many times is it executed
• are all expected headers sent to wiremock
• is a failed step logged correctly
• does a failed step cause the scenario to fail
• does the ignoreStepFailures flag work correctly
• ...

without having to create json-based scenarios for them.

Only having json-based scenarios to test with have two major drawbacks:

• these are integration tests, not unit-tests. Unit-tests are important to easily test edge-cases, and they allow you to find bugs much faster than an integration test, because their scope is not as broad.
• doing anything more refined than run-success testing, depends on the specifities of the generated reports. This ties the test too much into a scope that might change for totally unrelated reasons

Analysis of the code

As a base for further discussion, it's interesting to see what areas we could improve on. Very much my personal view and very open for discussion.

1. Single method with too many responsibilities

It handles:

• a scenario as a whole
• the loops of a scenario
• the steps in scenario
• the loops in a step
• the retry within those loops

At single step-execution level, it takes care of:

• setting up a step-execution specific logger
• initializing the step-execution state
• building a unique step-execution name

For every retry, it:

• evaluates the type of request to be performed
• delegates between these types
• does some type-specific pre-processing
• calls the type-specific executor
• logs the request execution
• logs the result of the execution
• updates the step- and scenario-execution state with the result

At the end of the retry-loop, it handles the assertions:

• asserts the results of the execution
• logs the assertions
• decides on whether to retry or not
• decides whether failed assertions cause the step to fail
• decides whether the loop continues

At the end of the scenario, it:

• stops the wiremockserver
• prints the assembled logs for the scenario

2. Some structural code issues, mostly a result of the many responsibilities:

• the method is some 330 lines long
• deep nesting (4 levels of for-loops)
• lots of early returns, which combined with the nesting, make it difficult to follow the flow
• lots of local variables, in a broad context, making it difficult to understand their purpose and impact
• direct manipulation of state managed by participant classes (more specifically, the stop-method on RestEndPointMocker.wireMockServer is called directly, not through RestEndPointMocker); this makes it diffult for that class to evolve (e.g. to make the wireMockServer mockable would require changes in two classes)

3. Reduced testability

• because all these responsibilities are handled in a single method, they cannot be tested individually
• a lot of direct coupling, using new or static factories, making the participating classes not mockable or verifiable
  • reportBuilder
  • reportResultBuilder
  • scenarioExecutionState
  • stepExecutionState
  • logCorrelationshipPrinter

[officiallysameer]

@BartRobeyns Thanks for putting this together. Do you have solution for this? We can tackle each point separately in different tickets.(One ticket will be too big to handle)

[BartRobeyns]

Where to begin? :)

Indeed it would be too big to handle; and I think we'd need some preparatory work before really diving in. Trying a first split of concerns (between the scenario and the steps), led me to the conclusion that the "lots of local variables" issue mentioned above needs to be handled first.

This is the set of variables that are shared between scenario-processing and step-processing:

• ScenarioSpec scenario
• ScenarioExecutionState scenarioExecutionState
• Step step
• RunNotifier notifier
• Description description
• ZeroCodeExecResultIoWriteBuilder reportBuilder
• ZeroCodeExecResultBuilder reportResultBuilder
• String host
• String port
• String applicationContext

Having so much shared state between scenario-handling and step-processing makes it more difficult to evolve and test individually. And it creates a 10-argument method call :) : StepExecutorImpl stepExecutor = new StepExecutorImpl(scenario, step, scenarioExecutionState, notifier, description, reportBuilder, reportResultBuilder, host, port, applicationContext);

Another issue that popped up is the use of logCorrelationshipPrinter's correlationId. It gets assigned a generated UUID in every step-iteration, and the one from the last iteration is used at the scenario-level to name to report-file. That name is important because it's used in the HTML-view to link the scenario to it's step-executions. If we want to split the step-processing from the scenario-handling, this should be changed.

How to solve these issues is very much a design decision, that only the experienced people can make. It requires understanding of the objectives and behaviour of logCorrelationShipPrinter, reportBuilder, reportResultBuilder, scenarioState, ... to be able to decide if and how to change the interaction with them.