LGTM

Given that this test is testing CLI functionality, but does not catch the bug in #32800

Is there an actual need to merge this? #32800 (comment) suggests rewriting to an API test (which won't do any testing here, as it's only consecutive calls to the same endpoint)

Shall we close this PR and remove the test?

If we have other test coverage for this endpoint I would agree we can just remove this test entirely.

It looks like we don't have any other tests for this endpoint, so this will need to be converted to api tests.

OK, this PR has been updated accordingly

Is there an equivalent test in cli to the one that is being removed?

The test should exist in e2e soon.

There is no equivalent in docker/cli right now. We're missing a lot of coverage in that repo. I opened docker/cli#201 as a tracking ticket for test cases we remove from this repo.

I don't think we should wait for those tests to exist before merging this and similar PRs. This test fails on the jenkins integration job because of changes to the CLI . Test coverage of the code in this repo remains the same after this change. We can't rely on tests in this repo to cover behaviour in another repo.

I don't think we should wait for those tests to exist before merging this and similar PRs.

I'm afraid I disagree.

This test fails on the jenkins integration job because of changes to the CLI.

So let's fix it instead of removing it.

We can't rely on tests in this repo to cover behaviour in another repo.

We need some way of making sure that the CLI works against the daemon. Currently it's impossible to add tests that verify this, and removing existing tests will make the situation even worse.

So let's fix it instead of removing it.

That's exactly what I did here! The fix is to make it an API test.
cc @andrewhsu since we've been talking about this.

We need some way of making sure that the CLI works against the daemon. Currently it's impossible to add tests that verify this, and removing existing tests will make the situation even worse.

Yes we do. It is the responsibility of docker/cli to make sure it works with the daemon.

The daemon should only be responsible for preventing regressions. Using a client binary is not the right way to do that, it hides regressions. We need to write API tests that check expected responses.

That said, the API test I wrote is terrible. I need to update it to expect an exact struct and raw bytes.

That's exactly what I did here! The fix is to make it an API test.

The old test checks the behavior of service ps with prefixes and multiple arguments. The replacement tests the ServiceInspect API method with a single full-length ID. I don't see how one is a substitute for the other.

The old test checks the behavior of service ps with prefixes and multiple arguments.

That behaviour is in docker/cli. We can not test it here. The only code in this repo that is being exercised by the removed tests is this one API call.

The test coverage of the system under test (the daemon) is exactly the same before and after this change (once I fix the assertion). The old tests checked behaviour outside of this repo, which is a bug in the tests, that is being fixed here.

I'm not comfortable removing the CLI test until there is a valid replacement in docker/cli.

I was wrong, there are actually unit tests covering this: https://github.com/docker/cli/blob/master/cli/command/service/ps_test.go

What value does this test provide? It's testing with an arbitrary frozen version of the CLI. We already know this test is failing with the latest version of the cli.

My first change was to update the test, but @thaJeztah correctly pointed out that it doesn't make sense to keep this test. It's not actually testing anything.

This isn't ready yet. I want to update the test to assert the entire struct matches the expect struct, instead of just comparing ID.

I've updated the test to compare the entire service instead of just the ID. This is ready for review again.

Failures look related;

22:40:24 ----------------------------------------------------------------------
22:40:24 FAIL: docker_api_swarm_service_test.go:682: DockerSwarmSuite.TestAPISwarmServicesInspectFull
22:40:24 
22:40:24 [d511d16ba1c2f] waiting for daemon to start
22:40:24 [d511d16ba1c2f] daemon started
22:40:24 
22:40:24 docker_api_swarm_service_test.go:697:
22:40:24     waitAndAssert(c, defaultReconciliationTimeout, d.CheckActiveContainerCount, checker.Equals, int(instances))
22:40:24 docker_utils_test.go:471:
22:40:24     c.Assert(v, checker, args...)
22:40:24 ... obtained int = 0
22:40:24 ... expected int = 2
22:40:24 
22:40:24 [d511d16ba1c2f] exiting daemon
22:40:49 

Hmm, yes it does, but I can't reproduce locally.

I don't see any errors in bundles/17.06.0-dev/test-integration-cli/d326d42de1f20/docker.log

ping @dperny @vdemeester could you have a look at this one; perhaps you have an idea

Something changed, I'm only seeing the PPC test failure which is unrelated.

oh! that's cool, I thought, well, let's try once more; looks like that resolved it, must've been fixed elsewhere then

This has been updated again to move the API test to the new integration/ suite