[NU-2140] Enricher mocking #7982
Conversation
raphaelsolarski
force-pushed
the
enricher-mocking
branch
from
b0ef3fe to
aa7c5d4
Compare
|
created: #8057 |
raphaelsolarski
force-pushed
the
enricher-mocking
branch
from
d8ce364 to
55326b6
Compare
raphaelsolarski
force-pushed
the
enricher-mocking
branch
from
dd4e3eb to
20f62a6
Compare
|
|
Not showing mockExpression for decision-table
raphaelsolarski
force-pushed
the
enricher-mocking
branch
from
f279350 to
fc313ac
Compare
There was a problem hiding this comment.
Why it is changed if we hide this feature behind feature flag?
| const EXPRESSION_TEXT_PATH = `${MOCKED_OUTPUT_IN_NODE_FIELD_NAME}.${MOCK_EXPRESSION_PARAMETER_NAME}.expression`; | ||
|
|
||
| const MOCK_EXPRESSION_HINT_TEXT = | ||
| "If provided the expression will be evaluated in tests instead of invoking real enricher behaviour (e.g. instead of calling a service/database).\n\n" + |
There was a problem hiding this comment.
It sounds a little bit complex, maybe simpler:
"If you provide this expression, the real service won't be invoked during tests. Instead, the result of the evaluation will be used."
| @@ -43,6 +48,8 @@ import shapeless.syntax.typeable._ | |||
|
|
|||
| object NodeCompiler { | |||
|
|
|||
| val MockExpressionParameterName: ParameterName = ParameterName("mockExpression") | |||
There was a problem hiding this comment.
Maybe let's use $mockExpression to avoid some collision
| @@ -1019,6 +1022,28 @@ class InterpreterSpec extends AnyFunSuite with Matchers { | |||
| result shouldBe Collections.singletonMap("bool", false) | |||
| } | |||
|
|
|||
| test("invokes enricher with mocked output in live mode") { | |||
There was a problem hiding this comment.
Hmm, I have two problems with this:
- By reading the test case title I don't know what do you expect. The real enriched should be invoked or no?
- I see that the real enricher wasn't invoked. Instead the mocked value was returned. We wanted to mock enrichers only in tests :) Maybe I misunderstand something?
| @@ -208,6 +209,15 @@ object node { | |||
| override def parameters: List[NodeParameter] = service.parameters | |||
| } | |||
|
|
|||
| sealed trait EnricherMockedOutput | |||
There was a problem hiding this comment.
When we talked about that I rather thought about something like:
EnricherTestingConfiguration(componentBehaviour: EnricherTestingBehaviour)
sealed trait EnricherTestingBehaviour
object EnricherTestingBehaviour {
case class ReturnMockedValue(outputValueExpression: Expression) extends EnricherTestingBehaviour
case object InvokeRealService extends EnricherTestingBehaviour
}
and having InvokeRealService as a default.
Thanks to EnricherTestingBehaviour envelope, we could add something more next to componentBehaviour.
But after we talk about further changes around testcases, I think that we should go into totally different, YAGNI approach and not invent this structure at all, because it shows that we have to be prepared for many evolutions of this structure.
BTW, I now see that this architectural decision about keeping test cases data in scenario graph will cause many scenario migrations which will be shown in the scenario history.
LSS: Let's remove this sealed trait.
|
|
||
| return ( | ||
| <ExpressionTestResults fieldName={MOCK_EXPRESSION_PARAMETER_NAME} resultsToShow={testResultsState.testResultsToShow}> | ||
| <EditableEditor |
There was a problem hiding this comment.
Currently, we are in the middle in the migration to Json template | Expression combination of editors in almost all places. So far, we did it conservatively, only in places where at the end, json is processed somehow, but there is the plan to use this approach almost everywhere. Let's use this combination here as well.
| ): NodeCompilationResult[compiledgraph.service.ServiceRef] = { | ||
| compileService(n.service, ctx, Some(outputVar)) | ||
| ): NodeCompilationResult[EnricherCompilationResult] = { | ||
| val serviceCompilationResult = compileService(n.service, ctx, Some(outputVar)) |
There was a problem hiding this comment.
I think that we should have somethink like
(runtimeMode, mockedOutput) match {
case (Test, Some(...) => compileMockExpression
case _ => compileService
}
See next comments
Describe your changes
Checklist before merge