Thanks! Looks like there's some linting failures;

14:07:06 integration/internal/api/image/image.go:12:1:warning: exported function Save should have comment or be unexported (golint)
14:07:06 integration/internal/api/image/image.go:25:2:warning: 'if err != nil { return err }; return nil' can be simplified to 'return err' (S1013) (gosimple)
14:07:06 integration/plugin/authz/authz_plugin_test.go:344:13:warning: ineffectual assignment to err (ineffassign)
14:07:06 integration/util/requirement/requirement.go:14:29:warning: unnecessary conversion (unconvert)

Yes, I've just fixed those. For some reason, make validate hangs on my machine recently. It would be really great to have some documentation of exactly what tests are run by CI and how. I've had make validate pass in the past but CI's linting fail. I also have tested this PR successfully locally with something like DOCKER_GRAPHDRIVER=vfs TESTFLAGS='-test.run TestAuthZPlugin*' make test-integration.

#34911 should make it a lot more obvious about what is run by CI. Needs another LGTM to merge

I've had make validate pass in the past but CI's linting fail.

If you encounter this again please let me know

Thanks for porting this test suite. Mostly looks good, but I don't think integration/internal/api/ should exist. Most of these functions are ` line wrappers around the api client which just hides the context. We should be testing the API more directly, not wrapping our client library with these helpers.

The tests being ported are CLI tests. These wrappers homogenize the API client interface to make it behave more like the CLI interface and improve the porting experience.

In a few cases there is some logic that can be extracted into factories for building the options struct, or helpers for transforming the responses, but I don't think they should wrap the client.

Why not? The tests do not demand most of the functionality that the API offers and which would clutter the tests with irrelevant noise.

I think it's also better that we keep these as unexported. There should be very little re-use of these between test suites because each suite should be focused on just the component under test.

As the CLI -> API library is located in internal, it can't be used outside of the integration tests. My cursory grepping of the repository indicates that many CLI tests use a variety of commands that do not correspond directly to the specific functionality under test.

Do you have a document describing the integration-cli -> integration test migration plan?

Could you show me an integration-cli test suite you have ported that demonstrates how you'd like the port to be done and how to reduce the chance of defects during the port?

The tests being ported are CLI tests. These wrappers homogenize the API client interface to make it behave more like the CLI

Exactly, that's the problem.

The tests do not demand most of the functionality that the API offers and which would clutter the tests with irrelevant noise.

These are tests of the API. If you hide how the API is called the test loses a lot of value, because it's no longer clear what you are testing. The helpers may even silently break the test by masking the actual API behaviour. I agree that implementation details (like complex setup, and complex assertions) should be hidden from the testcase function, but the actual API call needs to be there, because that is what is being tested.

many CLI tests use a variety of commands that do not correspond directly to the specific functionality under test.

Another major problem that will hopefully be fixed in the integration/ suite.

Do you have a document describing the integration-cli -> integration test migration plan?

There are a couple, but I will admit they are probably not as thorough as they could be. It is hard to enumerate all the problems with the old suite.

• Replace integration-cli suite with API test suite #32866
• https://github.com/moby/moby/blob/master/TESTING.md
• https://forums.mobyproject.org/t/evolution-of-testing-moby/38

Could you show me an integration-cli test suite you have ported that demonstrates how you'd like the port to be done and how to reduce the chance of defects during the port?

https://github.com/moby/moby/blob/master/integration/service/inspect_test.go there is also a test for version, but it is a very simple case.

Testing plugins seems to be more involved because it requires a daemon to be started with specific flags (which is not true of most of the other API endpoints).

An important detail is that the API endpoint that is being tested is central to the test, and any setup/teardown is (as much as possible) extracted out into helpers with appropriately descriptive names. I think for the most part the tests here fit that criteria, the only different is the abstraction over the API calls.

If there are some common helpers that will be used between plugins, integration/plugin/internal might be a good place for them.

The plugin tests are also a very different case because they are actually testing API calls into other components. Some of these helpers for plugins are appropriate, but if we expose them to all of integration/ they will be re-used incorrectly.

The ideal way to test these plugins would really be to re-use the test suite for the actual component. If we had the tests annotated correctly we could spin up a daemon that has the plugin loaded and then trigger the component test suite against that daemon. With the right test requirements/skips setup we wouldn't have to write a second test suite. We would have just a couple extra test cases for the "disallowed by plugin" case, which would get skipped when run against a non-plugin daemon.

I think that's probably unlikely to happen, so this suite is necessary.

It sounds like you have a lot of ideas for how moby/moby integration tests should work in the future. I opened this PR as it is all but required to satisfactorily test #33658. The goal here is reuse of existing test functionality to expand the authz test suite in a future-friendly way. I anticipate that this process of porting CLI tests to be independent of docker/cli will be required by other contributors who need to modify tests or contribute new tests. To that end, my goal is to make it as easy as possible to port CLI tests to be independent of docker/cli. This has several benefits:

1. integration-cli is drawn down and eliminated sooner as porting is easy
2. dependence on docker/cli is eliminated sooner
3. almost any contributor can port tests as the process is largely mechanical
4. incremental quality improvement is realized
5. the correspondence between CLI-esque and API use is immediately visible
6. any later SDK or client library author can see exactly and immediately the invocations necessary to achieve a CLI-like result
7. command executions are now named with symbols so they can be easily changed in the future

I think it's great that we have some ideas about how to improve the test suite in the future. In the present, having a mostly empty 'new' test suite and a large but deprecated 'old' test suite is negative for the project's health.

The existing tests are CLI tests (written as such) even if they should be testing the API or we want equivalent API tests. Actually testing the API directly will almost certainly look different from the current CLI tests and has its trade-offs. Couching CLI tests in the guise of API tests doesn't actually improve the tests but porting CLI tests to use the API under the hood does deliver the above benefits. Additionally, it seems to me that an ideal test suite would allow both 'low-level' API-style tests and 'high-level' CLI-style tests. As you note, some functionality like the plugins system greatly benefits from high-level tests.

It seems to me that any large scale improvement of the test suite should be done incrementally. Demanding that tests directly ported from integration-cli conform to some unwritten 'improved' specification seems counter-productive. The port itself (including functions that are 1:1 with the CLI) is valuable and speeds progress toward your long-term goals without demanding test rewrites by contributors or blocking other work. At a later stage, the CLI tests that have been ported can be rewritten or fixed up in uniform and well-defined stages as necessary.

The ideal way to test these plugins would really be to re-use the test suite for the actual component. If we had the tests annotated correctly we could spin up a daemon that has the plugin loaded and then trigger the component test suite against that daemon. With the right test requirements/skips setup we wouldn't have to write a second test suite. We would have just a couple extra test cases for the "disallowed by plugin" case, which would get skipped when run against a non-plugin daemon.

I don't believe that design is feasible as these plugin tests target the plugin system's functionality that extends beyond a single plugin or accept/reject behavior. Examine the tests for daemon start failure or the instrumentation of authz v1 plugins in this PR for evidence.

Test helpers which accept t *testing.T as the first argument instead of returning error are also really nice. If we keep some of these helpers I would suggest passing in testing.T, which will help keep the tests readable and not distract from the relevant lines in the test.

By not passing t *testing.T to the CLI analogs we retain the ability to test for expected failure.

I have rebased, squashed, and made the changes you requested. I strongly disagree with this design.

Please review and merge ASAP.

So when can this be merged?

Needs another LGTM