Fix error in CatalogITest

[sleberknight]

After importing from rickfast/consul-client, there is one test that "fails": CatalogITest#shouldGetNodeInCallback.

There is actually a PR in the original repository that fixes this issue: Fix breaking test and upgrade TestContainers for M1 support

The text of that PR is repeated here, in case it ever is deleted, etc. It is:

shouldGetNodeInCallback() in CatalogITest was using createAutoDeregisterServiceId() which adds to the list of agent registered services which is attempted to be deregistered in cleanup. The /catalog/services endpoint returns an aggregated list of services registered with each agent across the data center. The /agent/services endpoint will only return services registered against the specific local agent with which you are communicating. This test was not reliable in this regard as the registered catalog service was not found in the agent list of registered services and erroring.

So, the fix is to change the test to create a UUID (not use createAutoDeregisterServiceId()) for the serviceId.

Thus, it was not the test that failed, it was the test cleanup in the @AfterEach method in BaseIntegrationTest. To prevent this same problem from occurring in the future, we could modify BaseIntegrationTest so that the after method looks like:

    // need to add a Logger
    private static final Logger LOG = LoggerFactory.getLogger(BaseIntegrationTest.class);

    @After
    public void after() {
        try {
            deregisterServices.forEach(client.agentClient()::deregister);
        } catch (ConsulException e) {
            LOG.warn("Unable to degister service: {}", e.getMessage());
        }
        deregisterServices.clear();
    }

Basically, it catches failed attempts to de-register and logs them as warnings. A reason in favor of doing this is that the test did not fail, so we don't want test cleanup to cause test failures. an argument against doing this, is that the test used setup code that it should not have (i.e. using createAutoDeregisterServiceId()) and therefore we want tests to fail. If we take the latter approach, then we should maybe add a hint before just throwing the exception, e.g.

    @After
    public void after() {
        try {
            deregisterServices.forEach(client.agentClient()::deregister);
        } catch (ConsulException e) {
            String message = String.format("Unable to degister service. The serviceId was created using" +
            " createAutoDeregisterServiceId, but maybe it should not have been." +
            " For example, when using the /agent/services endpoint, which will only" +
            " return services registered against the specific local agent with" +
            " which you are communicating. Message from ConsulException which includes the serviceId: [%s]",
            e.getMessage());
            LOG.warn(message, e.getMessage());
            throw new RuntimeException(message, e);
        } finally {
            deregisterServices.clear();
        }
    }

This way, when reviewing a "failed" test you should see this warning and can fix the test setup, which is probably the better way to do this.

Also, we can add an informational log message when the createAutoDeregisterServiceId() method is called:

    protected String createAutoDeregisterServiceId() {
        String serviceId = UUID.randomUUID().toString();
        LOG.info("Created auto-deregister serviceId {}", serviceId);
        deregisterServices.add(serviceId);

        return serviceId;
    }