authorjapps / authorjapps/zerocode
Improve testability of MultiSteps Scenario Runner
- Dominant language
- Java
- Stars
- 1k
- Forks
- 453
- Avg merge
- 7d 2h
- Merged PRs (30d)
- 5
Description
This technical debt issue is a result of a discussion in #263, recognizing the need for testability of the [MultiStepsScenario Runner](https://github.com/authorjapps/zerocode/blob/c22fa8d73e4d7b688015dc8dac8c0cb6e1a8703d/core/src/main/java/org/jsmart/zerocode/core/runner/ZeroCodeMultiStepsScenarioRunnerImpl.java).
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
Contributor guide
Assessment
This issue has not been assessed yet.