Turning StopMockServers() into a no-op causes problems

[frncmx]

Source of problem

In recent versions (v1.3.x) StopMockServers() changed behavior. The function effectively became a no-op call. I understand that we want to spare time via reusing - and not restarting - already running Pact servers. However, the way we currently achieve that goal causes problems for us.

Scenario

1) Dev adds some pacts. 1) Dev runs tests with Pact. => Tests pass. 1) Dev realizes a mistake and corrects Pact body. 1) Dev reruns the tests. => Pact fails on adding the modified pact.

Runnable example

file: main_test.go

package main_test

import (
    "math/rand"
    "net/http"
    "os"
    "testing"
    "time"

    "github.com/form3tech-oss/go-pact-testing/pacttesting"
    "github.com/pact-foundation/pact-go/dsl"
    "github.com/spf13/viper"
    "github.com/stretchr/testify/require"
)

func TestMain(m *testing.M) {
    code := m.Run()
    pacttesting.StopMockServers()
    os.Exit(code)
}

func Test(t *testing.T) {
    defer pacttesting.ResetPacts()

    // Dev works and changes expectations as iterates:
    expectedBody := changingRequirementsBetweenRuns()

    const provider = "api"
    interaction := dsl.Interaction{
        Request: dsl.Request{
            Method: http.MethodGet,
            Path:   dsl.String("/"),
        },
        Response: dsl.Response{
            Status: http.StatusOK,
            Body:   expectedBody,
        },
        Description: "foo",
        State:       "bar",
    }

    require.NoError(t, pacttesting.AddPactInteraction(provider, "client", &interaction))

    _, err := http.Get(baseURLOf(provider))
    require.NoError(t, err)
    require.NoError(t, pacttesting.VerifyAll())
}

func changingRequirementsBetweenRuns() int {
    rand.Seed(int64(time.Now().Nanosecond()))
    return rand.Int()
}

func baseURLOf(provider string) string {
    return viper.GetString(provider)
}

The above produces error on 2nd run: An interaction with same description ("foo") and provider state ("bar") but a different response body has already been used. Please use a different description or provider state, or remove any random data in the interaction. (I use random data here, but a developer editing a pact matcher between runs would be enough.)

Workaround: pkill -f pact-mock-service between runs. That also produces the following error - which does not fail the test, but it's noise - api pact server defined in [...]/pact/pids/pact-api-client.json with pid 860726 no longer responding. Will start a new one

Before the change

Our TestMain used to stop the Pact servers before exiting the test binary, i.e., after m.Run() returns. That helped us avoid the above error.

However, that also produced some noise, i.e., the same "no longer responding" error as above. Moreover, if there was a panic during the test run the test suite could not recover.

Conclusion

I believe keeping the Pact servers running between TestA and TestB (same binary, same run) is a good idea. However, I think it's a bad idea to leave Pact servers running after the test binary exits. That leaks the processes - let them run forever - and that clearly breaks our usual dev flow. Since the time we win with not stopping/starting the Pact servers between separate runs is negligible compared to the time it takes to edit and compile the code.

Also, the original implementation was fragile. It misbehaved after panic and it was not able to recover in the case when Pact became inresponsive (e.g., Webbrick broken pipe error.) I believe, ideally, I would run StopMockServers() before and after m.Run(). That would ensure that the test puts itself a known starting state (setup) and we would not leak processes most of the time. To achieve that, I would parse the files in ./pact/pids and remove them after a successful shutdown. The latter would avoid the "no longer responding" error case reported by Pact.

[markhowardform3]

This appears to be a bug in the pact-standalone ruby server (https://github.com/pact-foundation/pact-ruby). DELETE /interactions doesn't seem to work.

[markhowardform3]

I think there is still benefit in having an option to keep the servers alive - particularly if you are debugging code changes rather than pact - to be able to restart very quickly. Given the issues with delete interactions though perhaps this should be changed to optional rather than the default behaviour.

[markhowardform3]

Hopefully, the rust rewrite will be done soon so we don't have the hassle and delays with the ruby version: https://github.com/pact-foundation/pact-go/issues/142

[frncmx]

I think the intended use/flow is defined here: mock-service-usage

After suite: write pact file, stop mock service

Today we learned if we don't stop the Pact servers they don't dump their pacts to /target dir so there is nothing to publish to the Pact Broker.

I would suggest bringing back the old code, with the option to not stop the Pact servers if STOP_PACT_SERVERS env is set.

[frncmx]

Resolved via https://github.com/form3tech-oss/go-pact-testing/pull/20