containers
Support additional_contexts
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.
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?
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.
@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:
- Black wants to reformat some files. Some of those are not changed in this PR.
tests/normalize_service.pyis 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. - Test case
test_podman_compose_includefails, but it also fails onmasterbranch, 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.
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.
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.
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!
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.