Interesting that docker-py caught things, and our CI didn't; looks like we may be missing coverage? 🤔

also related;

• containerd integration: image delete should send image "untag" events and prometheus counters #43855

w.r.t. the failing docker-py test; do we need an integration test somewhere that would catch those in our CI (not depending on the docker-py tests)?

w.r.t. the failing docker-py test; do we need an integration test somewhere that would catch those in our CI (not depending on the docker-py tests)?

First the "correct" output would need to be defined, the output is mostly informational and events should be used for triggering actions. I would say this is another case where unless we want to clearly define the output, we shouldn't just write a test to capture what it currently does.

First the "correct" output would need to be defined, the output is mostly informational and events should be used for triggering actions.

So there's 2 things that stood out to me; first of all, but that may be related to how the docker-py code collects the events, the test-output showed various None in the result;

E   AssertionError: assert 'sha256:4a96d7fec90f38afb825c65f9c5b08c726879ddecca2cd8580b615c7557ea28e' in

[None, 
'sha256:ed5c69b7b7ccca09e74045e1fe59ca7f56f121f5d34e991758fb4e0461bc1fe3', 
None, 'sha256:cbad98579cb46fa4b855c6...fa0a096a22351020f9fcda6a846a041', 
None, 'sha256:7c0ffe5751238c8479f952f3fbc3b719d47bccac0e9bf0a21c77a27cba9ef12d', 
...]

Which made me wonder if we were producing events without an Actor. If that's the case, that would be a bug, and that's something we should catch in our tests.

I would say this is another case where unless we want to clearly define the output, we shouldn't just write a test to capture what it currently does.

That's a fair point; that said: we already have an "implicit" contract here. I think it's fine to have a test even if that tests the current "status quo" behavior. As long as the test clearly outlines that that's what it does.

We've honestly had too many cases where we did not have coverage, and had to depend on docker-py to tell us that we changed behavior. Was the docker-py failure always correct? No, but, specifically events are widely used (e.g. in compose, but also desktop), so getting notified of changes in behavior through a test that starts failing is good. At least in that case we can make a conscious decision if the change in behavior was intentional or unintentional. I'd rather have a test failing that needs updating, than having to discover later that we broke compose (or desktop).

As to "what do we want it to do?" The docker-py test doesn't look unreasonable; it checks that an unused image appears as event when pruning;

@requires_api_version('1.25')
class PruneImagesTest(BaseAPIIntegrationTest):
    def test_prune_images(self):
        try:
            self.client.remove_image('hello-world')
        except docker.errors.APIError:
            pass

        # Ensure busybox does not get pruned
        ctnr = self.client.create_container(TEST_IMG, ['sleep', '9999'])
        self.tmp_containers.append(ctnr)

        self.client.pull('hello-world', tag='latest')
        self.tmp_imgs.append('hello-world')
        img_id = self.client.inspect_image('hello-world')['Id']
        result = self.client.prune_images()
        assert img_id not in [
            img.get('Deleted') for img in result.get('ImagesDeleted') or []
        ]
        result = self.client.prune_images({'dangling': False})
        assert result['SpaceReclaimed'] > 0
        assert 'hello-world:latest' in [
            img.get('Untagged') for img in result['ImagesDeleted']
        ]
        assert img_id in [
            img.get('Deleted') for img in result['ImagesDeleted']
        ]

OK; let's bring this one in, but we should create a tracking ticket to have a test.