spring-cloud / spring-cloud-contract

Support for Consumer Driven Contracts in Spring
https://cloud.spring.io/spring-cloud-contract
Apache License 2.0
720 stars 440 forks source link

WireMock is not reset before each test #1027

Closed kiesslingh closed 5 years ago

kiesslingh commented 5 years ago

We are using Cloud Contract 2.1.1.RELEASE on Spring 2.1.3.RELEASE.

With the WireMock integration '@AutoConfigureWireMock(port = 0)', Cloud Contract does not reset WireMock mappings before each test. We solved this by defining our own 'WireMockTestExecutionListener', but as this controverts test isolation, we would consider this omission a bug. If it actually is intentional what would be the rationale?

OlgaMaciaszek commented 5 years ago

@kiesslingh what is your usecase exactly? The mappings usually should reflect the producer app and also are stateless, so I'm not sure what improvements refreshing them with every test would provide?

kiesslingh commented 5 years ago

@OlgaMaciaszek Thanks for your reply. I guess the central characteristic of our usecase is that we are not stubbing a specific service, but a synthetic service that replays certain typical behavioral patterns of services that our component calls. So every test may use a stubbed server behaving quite differently from those in other tests, and still it makes perfect sense to put these tests in a single test class, because all of them test a specific feature of our own component, not one of the backend services.

To be more concrete: We're talking about a middleware server that forwards inward calls to different backend services. And we have to test the way in which the inward calls are mapped to backend calls in an abstract manner, as we don't want our tests rely on some idiosyncrasies of the backend services. Plus, these backend services typically are not available in the test environment anyway.

But apart from this usecase, is resetting and re-stubbing WireMock expensive? I didn't have that impression, but if it is, maybe it would be an idea to provide @AutoConfigureWireMock with an additional parameter, similar to @DirtiesContext(classMode = ClassMode.AFTER_EACH_TEST_METHOD), for controlling the reset? (@DirtiesContext, BTW, is really expensive: Our tests take 5 seconds more each when using that option.)

frzme commented 5 years ago

We also do the same thing as @kiesslingh. Our current workaround is to just call WireMock.reset() before each testcase. This also allow verification of the number of invocations.

kiesslingh commented 5 years ago

Actually - don't know whether this fits your usecase exactly -, we found a pretty good workaround: We use @TestExecutionListeners, and have the listener do the reset. We have defined our own test annotation that collects all the Spring test annotations that we use, and which also defines the TestExecutionListener, so this is very unobtrusive. Still, I wondered whether it'd be a good idea to support this directly with the WireMock integration

OlgaMaciaszek commented 5 years ago

@kiesslingh thanks, I sort of understand your scenario, but then usually the stubs under specific ivy notation will be generated once and look the same and other stubs under a different ivy location would usually be configured for a different test (for example in the @AutoconfigureWiremock annotation). Do you keep rebuilding the contracts to generate different stubs under the same ivy notation?

spring-projects-issues commented 5 years ago

If you would like us to look at this issue, please provide the requested information. If the information is not provided within the next 7 days this issue will be closed.

spring-projects-issues commented 5 years ago

Closing due to lack of requested feedback. If you would like us to look at this issue, please provide the requested information and we will re-open the issue.

kiesslingh commented 5 years ago

@OlgaMaciaszek I don't know 'Ivy' but we're doing ordinary 'self-programmed' tests and use nothing else from Cloud Contract up to now except for the WireMock integration itself, which is to say, @AutoConfigureWireMock, a TestExecutionListener to reset it after each test, and nothing else. Which is, to say it clearly, already very useful. From what you write the WireMock use case you want to support is much more specific than that, so: Are we misusing it from your point of view?

Anyway, maybe we just live with having to do the reset ourselves now that we found a good 'fool proof' solution for that in TestExecutionListeners.

ugrepel commented 5 years ago

@OlgaMaciaszek closing this was pretty fast, @kiesslingh responded just one day late.

I second non-clearing of the mappings as a bug, or at least as a feature that the @AutoConfigureWireMock annotation should support with an optional parameter.

Why do you say that each stub registered for one address always reacts the same for each test? Such a stub is a mock, which usually is tailored for a single test. Returning test specific data without really looking into the details of the request. It is not a full simulator.

Also the absense of a mock could be something that you want to test as well (failure behavior etc.), and a previously run test that just leaves a mapping lying around counteracts such test scenarios.

OlgaMaciaszek commented 5 years ago

@ugrepel it's done by a bot. Reopening it.

OlgaMaciaszek commented 5 years ago

Looks like a bug. We'll work on it.

fdw commented 5 years ago

We're also bitten by this bug. Is there anything I can help to fix this?

marcingrzejszczak commented 5 years ago

Should be fixed in the latest snapshots of 2.1.x and master branches (versions 2.1.4.BUILD-SNAPSHOT and 2.2.0.BUILD-SNAPSHOT)

belaaiza commented 4 years ago

Is any configuration required?

I have a class with two tests, the first one with a stub and the second with no stubs.

When I run WireMock.listAllStubMappings(); on the test with no stubs, the result shows one, which is from the previous test. If the mappings are being reset, shouldn't the WireMock.listAllStubMappings(); result be empty?

marcingrzejszczak commented 4 years ago

We're resetting after test classes not tests

kiesslingh commented 4 years ago

We're resetting after test classes not tests

Sorry, but this won't cut it. The level of isolation with tests is single tests, not test classes.

marcingrzejszczak commented 4 years ago

I think i care to disagree. However you can write your own test listener that can reset the stubs

kiesslingh commented 4 years ago

Isolation on test level was what this whole issue was about. If you don't have that isolation, test execution sequence will not be arbitrary (which it is in principle with JUnit), not to speak of test execution parallelization.

marcingrzejszczak commented 4 years ago

Can you create a simple sample that replicates this problem? I guess it will be easier to work on something tangible.

kiesslingh commented 4 years ago

We have lots of examples - unfortunately they are pretty domain-dependent and thus not that easy to understand. But in general, if you, for example, mock a response for a certain endpoint, and that response varies from test to test, that's a simple use case.

marcingrzejszczak commented 4 years ago

So you have to mock it anyways per test so you override the stubs of that wiremock server. I really don't understand the problem. It will be much easier if you provide a simple sample.

kiesslingh commented 4 years ago

Well, then, how about when you want the reaction checked, of your client, when the endpoint doesn't exist? So, in your case you have to explicitly delete the endpoint during test setup, in one way or the other. Of course, you can do that, but that's what test isolation is all about: what is commonly called closed-world assumption - the idea that whatever you don't explicitly specify in the test setup is not the case. It facilitates test setup. I can't explain it in any other way.

marcingrzejszczak commented 4 years ago

@kiesslingh you've convinced me. With https://github.com/spring-cloud/spring-cloud-contract/issues/1286 I've added a flag that allows you to reset stubs after each test mode

/*
 * Copyright 2013-2019 the original author or authors.
 *
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 *
 *      https://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */

package org.springframework.cloud.contract.wiremock;

import com.github.tomakehurst.wiremock.WireMockServer;
import com.github.tomakehurst.wiremock.client.WireMock;
import org.junit.FixMethodOrder;
import org.junit.Test;
import org.junit.runner.RunWith;

import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.beans.factory.annotation.Value;
import org.springframework.boot.test.context.SpringBootTest;
import org.springframework.boot.test.context.SpringBootTest.WebEnvironment;
import org.springframework.test.context.junit4.SpringRunner;
import org.springframework.web.client.RestTemplate;

import static org.assertj.core.api.BDDAssertions.then;

@RunWith(SpringRunner.class)
@SpringBootTest(classes = WiremockTestsApplication.class,
        properties = { "app.baseUrl=http://localhost:${wiremock.server.port}",
                "wiremock.reset-mappings-after-each-test=true" },
        webEnvironment = WebEnvironment.NONE)
@AutoConfigureWireMock(port = 0)
@FixMethodOrder
public class AutoConfigureWireMockWithResetAfterEachTestApplicationTests {

    @Autowired
    private WireMockServer wireMockServer;

    @Value("localhost:${wiremock.server.port}")
    private String hostname;

    @Test
    public void _01_test() throws Exception {
        this.wireMockServer.givenThat(WireMock.get("/should_register_mapping")
                .willReturn(WireMock.aResponse().withBody("bar")));

        String result = new RestTemplate().getForObject(
                "http://" + this.hostname + "/should_register_mapping", String.class);

        then(result).isEqualTo("bar");
    }

    @Test
    public void _02_test() throws Exception {
        String result = new RestTemplate().getForObject(
                "http://" + this.hostname + "/should_register_mapping", String.class);

        // taken from test/resources/mappings/resource-without-content-type.json
        then(result).isEqualTo("Hello World");
    }

    @Test
    public void _03_test() throws Exception {
        WireMock.givenThat(WireMock.get("/should_register_mapping")
                .willReturn(WireMock.aResponse().withBody("bar")));

        String result = new RestTemplate().getForObject(
                "http://" + this.hostname + "/should_register_mapping", String.class);

        then(result).isEqualTo("bar");
    }

    @Test
    public void _04_test() throws Exception {
        String result = new RestTemplate().getForObject(
                "http://" + this.hostname + "/should_register_mapping", String.class);

        // taken from test/resources/mappings/resource-without-content-type.json
        then(result).isEqualTo("Hello World");
    }

}

That should be backward compatible and should solve your issue.

kiesslingh commented 4 years ago

Excellent, thanks! Do you already know which release this will be in?