causify-ai
Come up with a testing approach for release flow
As per https://github.com/causify-ai/helpers/pull/590#pullrequestreview-2792676001 when we improve / refactor critical code like the docker flow we don't have a good way to test it, besides running it in production.
We are going to have soon a way to run tests outside a container, which will make it possible to run tests for this part of the repo. The next problem is how to test without actually running? An idea is to check that, under certain mocking, the sequence of system calls generated is the expected one.
FYI @heanhsok @dremdem @samarth9008
@sandeepthalapanane let's make a proposal on how to test it first.
Some related ideas are for https://docs.google.com/document/d/1xPgQ2tWXQuVWKkGVONjOGd5j14mXSmGeY_4d1_sGzAE/edit?tab=t.0#heading=h.1baftm82yypg
As per your suggestions above, we could use unittest.mock to test the Docker release workflow without requiring actual Docker operations and to mock external dependencies
Code Structure
-
Mock Infrastructure
- Mock Docker client interactions
- Mock system calls (
hsystem.system,hlitauti.run) - Mock registry client for AWS ECR and DockerHub
- Mock environment variables
-
Test Categories
a. Command Generation Tests
- Check that the correct sequence of commands is generated for different scenarios
- Test multi-architecture build commands
- Validate registry-specific commands
b. Error Handling Tests
- Test unsupported registry scenarios
- Verify version validation
- Test system error propagation
The plan looks reasonable. I think you understood the issue.
The goal is to ensure that by mocking everything that needs to be mocked, we can get a sequence of commands that is the one expected. After testing end-to-end with the mocks, we can unit test "deterministic" chunks of code by extracting code that doesn't interact and just testing it in isolation.
My proposal is to test a single function, showing how things will work and after that PR start expanding the coverage as we go.
@heanhsok, any thoughts?
The plan looks reasonable. I think you understood the issue.
The goal is to ensure that by mocking everything that needs to be mocked, we can get a sequence of commands that is the one expected. After testing end-to-end with the mocks, we can unit test "deterministic" chunks of code by extracting code that doesn't interact and just testing it in isolation.
My proposal is to test a single function, showing how things will work and after that PR start expanding the coverage as we go.
@heanhsok, any thoughts?
I think I will start by testing the docker_build_local_image (both single arch and multi-arch) since it's the first step in the docker release flow and covers many things.
I think I will start by testing the docker_build_local_image (both single arch and multi-arch) since it's the first step in the docker release flow and covers many things.
I don't have anything further to add right now, but this plan sounds good.
Found one interesting Docker feature: https://docs.docker.com/build/checks/ https://www.docker.com/blog/introducing-docker-build-checks/
Check a build without building To run build checks without actually building, you can use the docker build command as you typically would, but with the addition of the --check flag. Here's an example: docker build --check . Instead of executing the build steps, this command only runs the checks and reports any issues it finds. If there are any issues, they will be reported in the output.
It might be worth adding this to our tests and even including it in the Linter checks for Dockerfiles.
WDYT @samarth9008 ?
@dremdem
Can you figure out other who are involved in the issue? Could be GP or Heanh
https://github.com/causify-ai/helpers/issues/615#issuecomment-2850310356
@heanhsok Can you take a look pls.
- [x] PR comments https://github.com/causify-ai/helpers/pull/733/files
- [x] @heanhsok to review architecture / mocking approach
Good test implementation overall
Nits
Just one small nit (apart from the unresolved comments in the PR https://github.com/causify-ai/helpers/pull/733/files). Can we make the tests directory-agnostic (i.e., so the tests can be run from any directory)?
For example, running the tests from a non-root directory currently causes errors. We can use the with hsystem.cd(..) to temporarily change the context.
user_1042@4dfc7fb1ac6f:/app/helpers/test$ pytest test_lib_tasks_docker_release.py::Test_docker_build_local_image1::test_multi_arch1
...
test_lib_tasks_docker_release.py::Test_docker_build_local_image1::test_multi_arch1 (0.00 s) FAILED [100%]
======================================================================= FAILURES ========================================================================
____________________________________________________ Test_docker_build_local_image1.test_multi_arch1 ____________________________________________________
Traceback (most recent call last):
File "/usr/lib/python3.12/unittest/case.py", line 58, in testPartExecutor
yield
File "/usr/lib/python3.12/unittest/case.py", line 634, in run
self._callTestMethod(testMethod)
File "/usr/lib/python3.12/unittest/case.py", line 589, in _callTestMethod
if method() is not None:
^^^^^^^^
File "/app/helpers/test/test_lib_tasks_docker_release.py", line 200, in test_multi_arch1
hltadore.docker_build_local_image(
File "/venv/lib/python3.12/site-packages/invoke/tasks.py", line 138, in __call__
result = self.body(*args, **kwargs)
^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/app/helpers/lib_tasks_docker_release.py", line 384, in docker_build_local_image
_prepare_docker_ignore(ctx, docker_ignore)
File "/app/helpers/lib_tasks_docker_release.py", line 51, in _prepare_docker_ignore
hdbg.dassert_path_exists(docker_ignore)
File "/app/helpers/hdbg.py", line 756, in dassert_path_exists
_dfatal(txt, msg, *args, only_warning=only_warning)
File "/app/helpers/hdbg.py", line 142, in _dfatal
dfatal(dfatal_txt)
File "/app/helpers/hdbg.py", line 71, in dfatal
raise assertion_type(ret)
AssertionError:
################################################################################
* Failed assertion *
Path '/app/helpers/test/devops/docker_build/dockerignore.dev' doesn't exist!
################################################################################
================================================================== slowest 3 durations ==================================================================
0.01s call helpers/test/test_lib_tasks_docker_release.py::Test_docker_build_local_image1::test_multi_arch1
0.00s setup helpers/test/test_lib_tasks_docker_release.py::Test_docker_build_local_image1::test_multi_arch1
0.00s teardown helpers/test/test_lib_tasks_docker_release.py::Test_docker_build_local_image1::test_multi_arch1
================================================================ short test summary info ================================================================
FAILED test_lib_tasks_docker_release.py::Test_docker_build_local_image1::test_multi_arch1 - AssertionError:
################################################################################
* Failed assertion *
Path '/app/helpers/test/devops/docker_build/dockerignore.dev' doesn't exist!
################################################################################
Some further improvements
- The tests primarily cover happy paths. Including edge cases and error scenarios could be beneficial, as it would improve the coverage
- Do a dry run to check that the commands being executed are valid and won't cause errors at runtime (perhaps the Docker build checks feature can help with that)
WDYT? @gpsaggese @dremdem
Feel free to address just the Nits first @sandeepthalapanane
Good test implementation overall
Nits
Just one small nit (apart from the unresolved comments in the PR https://github.com/causify-ai/helpers/pull/733/files). Can we make the tests directory-agnostic (i.e., so the tests can be run from any directory)?
For example, running the tests from a non-root directory currently causes errors. We can use the
with hsystem.cd(..)to temporarily change the context.user_1042@4dfc7fb1ac6f:/app/helpers/test$ pytest test_lib_tasks_docker_release.py::Test_docker_build_local_image1::test_multi_arch1 ... test_lib_tasks_docker_release.py::Test_docker_build_local_image1::test_multi_arch1 (0.00 s) FAILED [100%] ======================================================================= FAILURES ======================================================================== ____________________________________________________ Test_docker_build_local_image1.test_multi_arch1 ____________________________________________________ Traceback (most recent call last): File "/usr/lib/python3.12/unittest/case.py", line 58, in testPartExecutor yield File "/usr/lib/python3.12/unittest/case.py", line 634, in run self._callTestMethod(testMethod) File "/usr/lib/python3.12/unittest/case.py", line 589, in _callTestMethod if method() is not None: ^^^^^^^^ File "/app/helpers/test/test_lib_tasks_docker_release.py", line 200, in test_multi_arch1 hltadore.docker_build_local_image( File "/venv/lib/python3.12/site-packages/invoke/tasks.py", line 138, in __call__ result = self.body(*args, **kwargs) ^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/app/helpers/lib_tasks_docker_release.py", line 384, in docker_build_local_image _prepare_docker_ignore(ctx, docker_ignore) File "/app/helpers/lib_tasks_docker_release.py", line 51, in _prepare_docker_ignore hdbg.dassert_path_exists(docker_ignore) File "/app/helpers/hdbg.py", line 756, in dassert_path_exists _dfatal(txt, msg, *args, only_warning=only_warning) File "/app/helpers/hdbg.py", line 142, in _dfatal dfatal(dfatal_txt) File "/app/helpers/hdbg.py", line 71, in dfatal raise assertion_type(ret) AssertionError: ################################################################################ * Failed assertion * Path '/app/helpers/test/devops/docker_build/dockerignore.dev' doesn't exist! ################################################################################ ================================================================== slowest 3 durations ================================================================== 0.01s call helpers/test/test_lib_tasks_docker_release.py::Test_docker_build_local_image1::test_multi_arch1 0.00s setup helpers/test/test_lib_tasks_docker_release.py::Test_docker_build_local_image1::test_multi_arch1 0.00s teardown helpers/test/test_lib_tasks_docker_release.py::Test_docker_build_local_image1::test_multi_arch1 ================================================================ short test summary info ================================================================ FAILED test_lib_tasks_docker_release.py::Test_docker_build_local_image1::test_multi_arch1 - AssertionError: ################################################################################ * Failed assertion * Path '/app/helpers/test/devops/docker_build/dockerignore.dev' doesn't exist! ################################################################################
It's not the tests that are failing but the hltadore.docker_build_local_image() function is failing. Do you want me to change the directory for every test before running the tested function?
Some further improvements
- The tests primarily cover happy paths. Including edge cases and error scenarios could be beneficial, as it would improve the coverage
I did check the coverage and covered as many as I could with mocking in the final pull request. I will take a look to see if I have missed any error scenarios. The coverage is around 80-85%, and I think the coverage report had the lines of code that I was not testing. I checked line by line and made sure to include and omit if it's not possible to mock or test.
- Do a dry run to check that the commands being executed are valid and won't cause errors at runtime (perhaps the Docker build checks feature can help with that)
I did take a look at Docker build checks; it's currently in the development stage, not completely ready. The features are limited, and probably, I will take a look again and see if we could implement it.
Can we make the tests directory-agnostic (i.e., so the tests can be run from any directory)?
Not sure it's worth doing right now.
I checked all the fast tests:
- In the test directory (
/app/helpers/test):- 24 failed, 1007 passed, 103 skipped, 54 deselected, 1 rerun in 59.02s
- In the root directory (
/app):- 1444 passed, 182 skipped, 91 deselected in 87.03s
It seems like some of our tests are not intended to run outside the root directory.
Do a dry run to check that the commands being executed are valid and won't cause errors at runtime (perhaps the Docker build checks feature can help with that)
Nice idea. I filed the similar issue: https://github.com/causify-ai/helpers/issues/673
It seems like some of our tests are not intended to run outside the root directory.
Maybe something like this will solve that and make things more explicit. WDYT?
git_root_dir = hgit.find_git_root()
with hsystem.cd(git_root_dir):
hltadore.docker_build_local_image(
self.mock_ctx,
self.test_version,
cache=False,
base_image=self.test_base_image,
poetry_mode="update",
multi_arch=self.test_multi_arch,
)
It seems like some of our tests are not intended to run outside the root directory.
Maybe something like this will solve that and make things more explicit. WDYT?
git_root_dir = hgit.find_git_root() with hsystem.cd(git_root_dir): hltadore.docker_build_local_image( self.mock_ctx, self.test_version, cache=False, base_image=self.test_base_image, poetry_mode="update", multi_arch=self.test_multi_arch, )
@dremdem could you check on this, so that we could either make changes or close this issue as completed.
Merged to master https://github.com/causify-ai/helpers/pull/770, https://github.com/causify-ai/helpers/pull/707, https://github.com/causify-ai/helpers/pull/733, https://github.com/causify-ai/helpers/pull/640, and https://github.com/causify-ai/helpers/pull/651.