Correctly handle scenarios with transformed stubs #2140
Conversation
…sformed stubs Revises SnapshotStubMappingPostProcessor to run stub transformers first. That way, if the transformer results in StubMapping requests being equal they are properly deduplicated or turned into scenarios. Cleans up process method to make clearer the three phases of processing. Additionally: - Converts SnapshotStubMappingPostProcessorTest.TEST_STUB_MAPPINGS to instance variable to avoid sharing mutable state between test cases which caused the suite to be order dependent. Fixes: wiremock#2137
|
Hey @tomakehurst gentle bump on this. This issue has my team maintaining a kludgy internal fork of Wiremock, I'd love to get this into the main repo :). |
|
Can we fix the build errors also? Then I'll take a look. |
|
@tomakehurst AFAICT there are codestyle errors only. |
Merging did actually result in introducing a bug, I've resolved it. All tests are passing now, and Spotless has been reapplied so this one should be good to go too. |
|
Thanks @gsmith85 , and sorry for messing up the merge |
|
@tomakehurst @oleg-nenashev are we a go here? Anything else you need form me to get this merged? |
There was a problem hiding this comment.
I had to sleep on it, and after all I think that the new behavior is wha8t we need for the real use-cases. There is a risk of slight performance degradation, but it repeatAsScenarios is rather an advanced feature so it should not impact many users.
Merging it towards 3.0 Beta and annotation as a breaking change is something I'm +1 for. Needs Tom ofc
|
|
||
| assertThat(actual, hasSize(2)); | ||
| assertThat(actual.get(0).getRequest().getUrl(), equalTo("/foo/transformed")); | ||
| assertThat(actual.get(1).getRequest().getUrl(), equalTo("/bar/transformed")); | ||
| } | ||
|
|
||
| @Test | ||
| public void processExtractsBodiesWhenMatched() { | ||
| public void | ||
| process_withShouldRecordRepeatsAsScenariosAndTransformer_runsTransformerBeforeScenarioProcessor() { |
There was a problem hiding this comment.
I'm finding these test names hard to make sense of, and they're inconsistent in style with the rest of the codebase. Please could we make them statements of outcome and camel cased?
There was a problem hiding this comment.
Apologies for going in a different direction with test naming here.
A bit on rationale, the pattern method_condition_expectedOutcome was chosen based on capturing the recommended components for unit test naming per Google Testing and specifically I've included outcomes in this context because documenting that aspect feels valuable. The behavior of the SnapshotStubMappingPostProcessor is a bit subtle, I want to make sure each test's goal is articulated so the reader isn't left attempting to parse the intended behavior from the assertion set.
All of this can be done in a fashion more aligned to the conventions of this project. I've switched over to a method<With>condition<Should>expectedOutcome format, so I think we're in good shape.
| final List<StubMapping> processedStubMappings = new ArrayList<>(); | ||
|
|
||
| // 1. Run any applicable StubMappingTransformers against the stub mappings. | ||
| ArrayList<StubMapping> transformedStubMappings = new ArrayList<>(); |
There was a problem hiding this comment.
This plus the next few lines could be done more neatly by mapping over a stream.
There was a problem hiding this comment.
If we're using Iterable as the type for stubMappings this actually may be worse from a readability perspective:
List<StubMapping> transformedStubMappings =
StreamSupport.stream(stubMappings.spliterator(), false)
.map(transformerRunner)
.collect(toList());Note that when using Iterables you must use StreamSupport to gain access to a Steam.
So what I'm proposing is we change the method signature of process as well to use Collection so you end up with:
public List<StubMapping> process(Collection<StubMapping> stubMappings) {
// 1. Run any applicable StubMappingTransformers against the stub mappings.
List<StubMapping> transformedStubMappings =
stubMappings.stream().map(transformerRunner).collect(toList());
...Collection is a bit more narrow than Iterable but I think a general enough abstraction for this API and appropriate for the semantics of stubMappings.
|
@tomakehurst I believe I've addressed your comments, appreciate the guidance on this one. Let me know if you need any other tweaks and I can be more responsive to help get this pushed over the line. Unrelated note: |
|
Thanks @gsmith85 ! |
Revises SnapshotStubMappingPostProcessor to run stub transformers first. That way, if the transformer results in StubMapping requests being equal they are properly
deduplicated or turned into scenarios.
Cleans up process method to make clearer the three phases of processing.
Additionally:
Fixes: #2137