Closed ncoghlan closed 7 years ago
@junaruga @hroncok I wasn't sure where or how to test this addition, so the PR doesn't currently include any new unit tests, it just adds a step to the custom echo file that prints out the new environment variable.
However, I'm wondering if it may make sense to consider moving the integration tests out of bash and into a framework like behave that can more easily capture the command output and make assertions about it.
@ncoghlan thank you for your pull-request. :) Sorry right now I have a few tasks that I have to. After that, I would take a look at this PR, and comment.
@ncoghlan Right now I checked your code and the document. It looks awesome. Thanks.
Where we need to test is adding your test to below file. Could you add your test for your code?
tests/test_custom.py
Because the integration test is not good and it is deprecated. we decided to update test files for pytest instead of the integration test tests/integration/run.sh
.
See https://github.com/sclorg/rpm-list-builder/issues/23
I'd run integration tests using pytest as well.
Thanks.
OK, I'll add a new test that runs the custom echo backend in a subprocess, and checks for the expected output.
Since it doesn't do any actual building, that should still be fast enough to be a reasonable unit test, and it will avoid having to design in any mocks.
I can also add an explicit test for #81 at the same time.
All right. Thank you.
Working on adding a test case for this, and it looks like there may be another test case which isn't reverting a monkeypatch correctly. My rationale for that is that when I do:
python3 -m pytest -s tests -k custom_build_hooks
to run just the new test case, then I get the expected behaviour (i.e. I can see the output from running with the echo.yml
configuration).
By contrast, running:
python3 -m pytest -s tests -k custom
fails, and the output suggests that the custom YAML commands aren't actually being executed at all, so the build step fails due to the download step failing to create the required directories.
As a first guess, https://github.com/sclorg/rpm-list-builder/blob/master/tests/test_custom.py looks it may be the source of the problem, as it includes a couple of lines like:
type(valid_custom).yaml_content = mock.PropertyMock(
return_value=content,
)
That skips reverting back to the original property descriptor after the test case is complete, which would explain the subsequent misbehaviour of testing a full custom build invocation.
I'll switch that over to using the monkeypatch
fixture (https://docs.pytest.org/en/latest/monkeypatch.html) so that it gets reverted automatically after the test case finishes.
The above change fixed the custom backend invocation, but there's a separate problem in the interaction with test_cli
, where that is somehow turning off the log capture for runner.invoke
@ncoghlan runner.invoke
does command-line parsing and callback invocation. There is a callback that will set the logging level to INFO
unless you pass the --verbose
flag.
I couldn't figure out what was going on with the failure to reliably capture output when using click.CliRunner
, so I abandoned that approach and switched to using a simple subprocess invocation instead.
However, I'll be submitting a couple of other PRs that tidy up test_custom
and test_cli
a bit, in an effort to make them more reliably side-effect free.
@khardix Aye, and even the normal behaviour when running the test standalone was to only capture INFO messages and not the results of running the YAML hooks. The weird interaction was that the test would end up capturing no output, and instead print everything directly to the terminal.
Between click & pytest I decided there were just too many moving parts to try to debug, when using a real subprocess instead makes it straightforward to completely avoid any potential for interference.
@junaruga I've added a test that runs a custom "echo" build in a subprocess, and checks for the expected messages reporting the custom directory and the package names.
Using a full subprocess will also allow it to be extended to cover #81, but I figure that change belongs in a separate PR that also updates the documentation.
@ncoghlan Thanks! So, after you squash to 1 or 2 commits as you like, I would approve and merge it.
Done! Also 😃 at the typo in the last pre-squash commit message, as it didn't occur to me what this command would actually do:
git commit -m "Replace other uses of `pwd`"
Thanks for the great work. Merged.
This allows configuration files and helper scripts to be located relative to the custom hook file that needs them.
For example, an uninstalled mock configuration file can be referenced as
mock -r ${CUSTOM_DIR}/custom-mock.cfg ...
.Closes #67