build: follow up fixes for build stream

[tonistiigi]

Addresses the follow-up comments in #231 (review)

For the content-trust update, I have no idea why the FROM images should be retagged by the builder. I would argue that this is wrong but made it match the non-stream behavior.

@dnephin

Signed-off-by: Tonis Tiigi [email protected]

[dnephin]

There's another one or two places where this buffer is printed I believe. I have the buffers fixed in #294.

[codecov-io]

Codecov Report

Merging #300 into master will decrease coverage by 0.01%.
The diff coverage is 58.82%.

@@            Coverage Diff             @@
##           master     #300      +/-   ##
==========================================
- Coverage    50.1%   50.09%   -0.02%     
==========================================
  Files         216      216              
  Lines       17700    17703       +3     
==========================================
- Hits         8869     8868       -1     
- Misses       8387     8391       +4     
  Partials      444      444

[thaJeztah]

ping @tonistiigi @dnephin what's the status on this one?

[dnephin]

I prefer the solution in #294

[tonistiigi]

I didn't notice that this got stuck. This is not a refactor but fixes build -q --stream not working at all. I recommend merging and picking to relevant docker-ce releases.

[andrewhsu]

@dnephin would it be possible to bring this PR in as a more isolated change and then refactored for #294 ?

[dnephin]

Sure, that's possible. This PR has no tests, so I'm not sure how confident we can be that it still fixes the problem.

[thaJeztah]

ping @tonistiigi is it possible to add a test for this PR?

[thaJeztah]

(or go for #294 if you think that's better)

[tonistiigi]

@thaJeztah This behavior should be tested with e2e/integration test. So not atm(or any more) afaik. I repeat that this is a fix for a bug still existing in 17.07+

[dnephin]

This behavior should be tested with e2e/integration test.

I disagree. The bug was not in the integration of components, it was in a single component, which means it can be unit tested, if the code is structured properly.

There is an e2e suite in docker/cli https://github.com/docker/cli/tree/master/e2e, but writing a e2e test for every minor bug is not a sustainable way of testing software.

[tonistiigi]

The bug is in no component. The bug is in the cli wrapper so the test needs to use the cli. Implying that these cli unit-tests with mocks actually catch any bugs is something I can not agree with and to my knowledge has become evident in almost any time cli and engine are combined in docker-ce (eg #654 #577). A sustainable way of testing software is to write tests that catch bugs, not ones that add coverage.

Feel free to close, as it shouldn't take 3 months to merge a fix for not printing io.Writer as a string.

[dnephin]

The bug is in the cli wrapper so the test needs to use the cli

I'm not sure what you mean by "cli wrapper". The bug is in the cli binary, which is a component of a larger system. This bug wasn't with the integration of components, it was internal to a single module, so doesn't need a binary with an API to exercise the bug.

cli unit-tests with mocks actually catch any bugs is something I can not agree

Unit tests will not catch integration errors, that's true. That's not their purpose. Their purpose is to validate that a unit behaves correctly. In this case the unit is a module in the cli repo.

Integration and e2e test are also important, but when every test is written in that style is causes many problems. Integration and e2e tests should test integration of components, and end user features, not internal logic of a module.

has become evident in almost any time cli and engine are combined in docker-ce

Yes, there are plenty of missing tests in docker/cli and moby/moby. Sometimes those holes in the test suite are discovered through integration tests, which is a good opportunity to write the missing unit tests. Many of the tests failures during integration are actually due to bad assumptions in the integration suite. Others failures are due to missing unit test coverage.

Considering that test coverage is barely 50% I'm actually surprised that we average fewer than 1 integration bug per release.

[tonistiigi]

I've added another commit to open Dockerfile from correct location.

Integration and e2e tests should test integration of components, and end user features, not internal logic of a module.

End user feature is broken. You can't build with certain flags.

I'm actually surprised that we average fewer than 1 integration bug per release.

The examples were from regressions in the old code that happened to have tests before 17.03. It is very hard to assume anything about the number of bugs in new features as like this one they are only discovered by manual testing after release is out.

[dnephin]

End user feature is broken. You can't build with certain flags.

Yes, every bug would impact some user feature, that's almost the definition of a bug. Are you arguing that there is no place for unit tests, and that every test must exercise the system end-to-end to be of any value? I don't really understand how anyone could make that argument.

It is very hard to assume anything about the number of bugs in new features as like this one they are only discovered by manual testing after release is out.

Which is exactly why I've been asked for test coverage, so we don't have to assume.

[tonistiigi]

Are you arguing that there is no place for unit tests, and that every test must exercise the system end-to-end to be of any value?

No. There are many cases where unit tests work great. When there is an isolated component with features specific to that component that can be tested (in most cases, you need both types). This is not a case in here as only thing this code does is take cli flags, process them and form cli output from daemon response. Taking a substep of that and creating mocks to provide an expected result just means that same code is written two times and doesn't provide any guarantees that it actually works or discovers bugs when some other component changes. I think we have plenty of data to support this.

Which is exactly why I've been asked for test coverage, so we don't have to assume.

If the tests are not actually testing anything the test coverage has no correlation to the number of bugs we ship.

I'm sorry for falling into this tirade, but I'm getting frustrated for the constant reports showing up from end users for different build features(because there have been many new features and refactoring prs). Almost all of them fall into a category that would have been impossible 6 months ago when we had tests for the actual features of the platform and never merged anything without a feature test and all of the test suites passing. Not to mention people making the changes actually tested them with code shipping with docker-ce before it reached the users. Something has to change and I'd rather make the problem more obvious than contribute to the misleading coverage report.

[dnephin]

When there is an isolated component with features specific to that component that can be tested

Unit tests do not test product features! They test units of code. If you are testing features then it's not really a unit test. So it does sound like you would only like tests that exercise end user features. While I agree it is important to have end-to-end feature tests, I think unit tests are equality important. They allow developers to get quick feedback on changes they've made, and give important signals about code structure and interfaces. Without unit tests the architecture can quickly devolve into a big ball of mud. The cost of units tests, both in developer time and infrastructure to run the tests is also much lower. There are plenty of other benefits to unit tests that can fill a book.

This is not a case in here as only thing this code does is take cli flags, process them and form cli output from daemon response.

That is true for some cli commands, however the build command has over 1200 lines of code in cli/command/build*. If this was as simple as translating command line flags into an API call I don't see why it would take 1200 lines. There are plenty of units in there to test. They may not be obvious right now, because of the code structure, but they are there.

Taking a substep of that and creating mocks to provide an expected result just means that same code is written two times and doesn't provide any guarantees that it actually works or discovers bugs when some other component changes.

That's right, it doesn't catch any regression caused by changes to the API. That is not the purpose of a unit test. There definitely need to be end-to-end tests as well.

There is no duplicate code. The fake is generally one or two lines that builds a static object. If the API was as simple as sending a static object then there wouldn't be much value in the API.

The guarantee that it provides is that the unit performs correctly given a specific input. This bug was not an integration bug. It can easily be reproduced with specific static input.

I think we have plenty of data to support this.

I haven't seen any data to support this. You mentioned two issues, so lets look at those 2 issues:

• cli/command/container: ensure channel is allocated #577 - had no test coverage in unit tests or e2e tests. This doesn't support your argument that unit tests with fakes are useless because there was no test coverage for the change in either suite.
• Fix behaviour of rmi -f with unexpected errors #654 - was a result of fixes #46 Always exit 0 when remove image with force option #160, which did have test coverage, but the coverage was changed to support the new behaviour. This was not actually a bug, but an incorrect feature. Again this doesn't support your case because the issue was not that tests didn't catch the problem, they did catch the problem, but we decided (in hindsight incorrectly) that we wanted the new behaviour.

If you have other cases that you believe demonstrate the issue, we should discuss this further.

I'm sorry for falling into this tirade, but I'm getting frustrated

I understand your frustration. We made a large change (splitting the repos) and never invested the effort to actually finish the transition. We are making progress in the right direction. We have an e2e test sutie in this repo which allows us to test against a real daemon, but there are still plenty of tests that need to be ported.

Something has to change and I'd rather make the problem more obvious than contribute to the misleading coverage report.

It sounds like your disagree with the testing philosophy used in this repository, and your protest is not writing a unit test for the bug. That is certainly your right, but I think it would also be expected that it would delay, and possibly even prevent the merging of this PR.

You could contribute an e2e feature test in e2e/image/build_test.go that tests the success path of using --stream. That wouldn't be contributing to the test suite that you feel is useless.

However, I still maintain that the bug this PR is fixing would have been caught by a unit test. The bug can be exercised without even hitting the API, so an e2e test to catch this issue does not make sense.

[tonistiigi]

If this was as simple as translating command line flags into an API call I don't see why it would take 1200 lines. There are plenty of units in there to test. They may not be obvious right now, because of the code structure, but they are there.

Absolutely, but I'm not trying to rewrite these 1200 lines. I'm trying to fix a bug. In first commit, the bug is printing io.Writer as a string. In second it is using a wrong variable in one line.

There is no duplicate code.

I didn't mean that the mock is a duplicate, but that the tests don't test a behavior but follow the implementation. This does not apply in cases where there is a testable unit, but cases, where there isn't one, but unit-test is used to cover a specific implementation line that changed.

For the data, I meant data on that our test coverage/quality has gone down. That is based recollection that every *.0 release since 17.06 has had some builder related bug. Just today I was looking at moby/moby#33974 (comment) . I don't know what is the issue there yet as more info is needed, but I do know that we don't have proper coverage about these cases (our previous exchange on that, that I lost moby/moby#34063 (review)). Some of these issues have fixes in cli, some don't. The bottom line issue is the same - that we don't have tests for user features.

Both of the issues you mentioned had plenty of coverage in moby test suite that used to test this code before 17.06. #577 especially, as it fixes a case that made docker run/attach completely unusable but nothing caught it in the review process of the cli repo.

You could contribute an e2e feature test in e2e/image/build_test.go that tests the success path of using --stream.

I'll do that.

[thaJeztah]

@tonistiigi have you had time to look at adding a test in e2e/image/build_test.go ?

[tonistiigi]

@thaJeztah I still plan to do it but don't know when. I didn't notice that there were no cases that cover building in e2e package yet, eg the file you referenced doesn't exist.

[dnephin]

Opened #709 which adds an e2e test for docker build using a directory as context.

[vdemeester]

ping @dnephin @tonistiigi what is the status here ? 👼

[dnephin]

Just needs a test