Support additional_contexts

[otto-liljalaakso-nt]

Fixes #762

[theyoyojo]

👍 I'm using this commit in my fork

[p12tic]

Thanks for the PR, looks great!

Could you add unit tests (pytests/ directory) and also an automatic runner for the docker-compose.yml that you've added. There are examples of such tests in tests directory now.

[otto-liljalaakso-nt]

I have added the automatic runner test in tests/. I also tried looking at unit tests, but I would need some guidance here.

This PR touches two functions: normalize_service() and build_one().

normalize_service() is called by some unit tests, but not in systematic fashion: The test files are called test_can_merge_build.py, which is about build key specifically, and test_can_merge_cmd_ent.py, which I do not really understand, but apparently is about command line arguments.

My question 1: Should I create a new file test_can_merge_additional_contexts.py and write my unit tests there, or what is the expectation here?

build_one() is not called by any unit test, and is a bit problematic for unit testing because it does filesystem access.

My question 2: Is the expectation that I should develop enough unit testing framework, with filesystem mocking etc., for build_one(), so I can unit test the changes in this pull request? I can create something, I am just not sure if assigning such task to first time, drive-by contributor is a good idea.

[tchernobog]

Hi @p12tic , we were also looking for this feature to be implemented, and it seems like @otto-liljalaakso-nt addressed partially your review comments.

Could you answer his question and move this forward?

[p12tic]

I'm sorry, I missed the comment by @otto-liljalaakso-nt.

normalize_service() is called by some unit tests, but not in systematic fashion: The test files are called test_can_merge_build.py, which is about build key specifically, and test_can_merge_cmd_ent.py, which I do not really understand, but apparently is about command line arguments.

I cleaned up normalize_service() tests. They are now in pytests/test_normalize_service.py. Indeed the tests were hard to understand, hopefully no more.

My question 1: Should I create a new file test_can_merge_additional_contexts.py and write my unit tests there, or what is the expectation here?

As of now the best place for normalize_service tests is in pytests/test_normalize_service.py.

build_one() is not called by any unit test, and is a bit problematic for unit testing because it does filesystem access.

Agreed. But I think the change is simple enough that we can leave it untested. I feel that the amount of effort needed to test build_one() is too high for the benefits.

My question 2: Is the expectation that I should develop enough unit testing framework, with filesystem mocking etc., for build_one(), so I can unit test the changes in this pull request? I can create something, I am just not sure if assigning such task to first time, drive-by contributor is a good idea.

Completely agreed. Lack of testing framework is problem of the project. Until there's one, the parts that are hard to test will unfortunately be left not tested.

[otto-liljalaakso-nt]

@p12tic I have updated tests/normalize_service.py to test this change. CI is showing red, as did pre-commit whose use is suggested by CONTRIBUTING.md. According to my analysis, none of the reported problems are caused by changes in this PR:

1. Black wants to reformat some files. Some of those are not changed in this PR. tests/normalize_service.py is changed in this PR, but the changes Black wants to apply are much wider than that. So I think those problems should not be fixed in this PR.
2. Test case test_podman_compose_include fails, but it also fails on master branch, so I do not think the problem should be fixed in this PR.

So think this is ready to merge, and those unrelated problems should be dealt with separately.

[otto-liljalaakso-nt]

If you prefer, I can add a commit that runs Black and applies the changes it wants to make. For the failing test case, I don't know what it is about.

[p12tic]

Thanks, everything looks good.

[p12tic]

The CI was broken during last few days due to some PRs that have been pushed a bit carelessly. This is now fixed. I will rebase the PR and merge it.

[p12tic]

If you prefer, I can add a commit that runs Black and applies the changes it wants to make.

I ran ruff format . after rebasing, no need to do anything. Thanks!

[otto-liljalaakso-nt]

If you prefer, I can add a commit that runs Black and applies the changes it wants to make.

I ran ruff format . after rebasing, no need to do anything. Thanks!

Great to hear that.

I would like to suggest reviewing pre-commit configuration and the suggestion to run that in CONTRIBUTING.md. It looks like pre-commit and CI are using different tools for the same purpose, leading to confusion like this.