Use mocking for Docker decorator tests

[malthe]

Previously, the tests would actually run a Docker container.

Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.

[ashb]

a) What's wrong with using an actual docker container for this test?
b) We are now testing the docker_execute function, rather than the code that the docker decorator injects.

So If we don't want to us a container (which I'm not sure of) we at least need to test the real code.

[malthe]

a) What's wrong with using an actual docker container for this test? b) We are now testing the docker_execute function, rather than the code that the docker decorator injects.

So If we don't want to us a container (which I'm not sure of) we at least need to test the real code.

This is just the decorator code. The assumption is that it uses the Docker operator which is tested elsewhere (I hope!)

[malthe]

Test failure seems unrelated.

[potiuk]

@ashb ?

[github-actions]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.

[malthe]

@ashb fwiw I still think mocking this is much better than calling out to the docker tool – also because this is what we do with all other operators.

[ashb]

The key difference here to my mind is that we can run the actual docker code without calling out to any external service -- so I'm wary of using mocks here as we run the risk that we are then testing the mock, not the actual code.

In this case, we are testing the docker_execute function in the test file, not the real code that the Docker decorator actually generates.

[malthe]

But isn't that always the assumption when we mock external dependencies – that we're doing it correctly. There is always that risk that the assumptions are wrong. But that would the hopefully be discovered during alpha/beta testing.

[potiuk]

But isn't that always the assumption when we mock external dependencies – that we're doing it correctly. There is always that risk that the assumptions are wrong. But that would the hopefully be discovered during alpha/beta testing.

Not really.

We rather follow the typical test pyramid:

This is just one incarnation of it: https://martinfowler.com/articles/practical-test-pyramid.html

• Plenty uf real unit tests
• Far less number of integration tests with actual local services including actual Docker and actual Kubernets tests - KiND based (those are special cases of Integration tests)
• Very few system tests (AIP-4 an AIP-47 are about fully automating those) talking to actual external services.

This is rather comprehensively described in https://github.com/apache/airflow/blob/main/TESTING.rst (all the test types we have are described there).

I personally think the "test pyramid" approach is far more useful than "only unit tests". I find the latter a bit too dogmatic and I think our integration tests and system tests play a role there (System tests especially for Providers in the future).

[malthe]

@potiuk that sounds reasonable so in this particular case, are you saying that we do want to actually test that we can run a container workload?

Just bear in mind that what motivated this was that an external container registry was unable to provide the container image in time so the test timed out.

[potiuk]

@potiuk that sounds reasonable so in this particular case, are you saying that we do want to actually test that we can run a container workload?

Just bear in mind that what motivated this was that an external container registry was unable to provide the container image in time so the test timed out.

Yeah. Those external images fail occassionally - but not often enough to make it a problem. Our tests will anyhow refuse to work if a number of images are not available.

We actually have quite a good sytem in-place to prevent from depending on "external images" to break our tests - rather than rely on those external images we either build, or simply push such an image to our Github Registry - this way we are free from dependence of the external images in a location that is not ours and speed up the builds as Github Registry is generally "close" and "more stable" for Github actions.

Also it has the nice benefit that we do not fall into DockerHub limits of pulling images (DockerHub images have very aggressive rate limiting for unauthenticated pulls (based on outgoing IP address).

Github Actions Public runners are exempted from it agreement of DockerHub and GitHub, similarly as Apache DockerHub Images (ASF has an agreement with DockerHub about it) but our self-hosted runners pulling regular "DockerHub" images are only protected by the limit for authenticated users (we do authenticcate in our self-hosted runners). Those rate limits are better but still we might be rate limited at times.

GHCR.IO images have no such limit - you can see example image here, but there are few more: https://github.com/orgs/apache/packages?tab=packages&q=airflow-trino.

In this case I think we are using quay.io/bitnami/python:3.9 for that reason (the official image of Python might make us hit rate limits). So the solution here is to push "airlfow-python:3.9" image to ghcr.io and use it from there. This will be faster and more reliable. I will push it in a moment and you might switch to it in a new PR.

[ashb]

Couldn't we test against the Airflow docker image we build/run tests in already?

[potiuk]

Couldn't we test against the Airflow docker image we build/run tests in already?

I think is quite heavy for the tests and have some side effects (for example we have a custom entrypoint)- this image to test here is just a plain python image.

I just pushed an alpine based one: https://github.com/orgs/apache/packages/container/package/airflow-python

This was the command to prepare it (@malthe you can add it in a comment next to the test that this is the command we used to get it:

dpcker pull python:3.9-alpine && \
echo "FROM python:3.9-alpine" | docker build --label org.opencontainers.image.source="https://ghcr.io/apache/airflow" -t ghcr.io/apache/airflow-python:3.9-alpine - && \
docker push ghcr.io/apache/airflow-python:3.9-alpine