search

dogecoin / docker

Dogecoin Core on Docker
MIT License
27 stars 18 forks source link

Add test coverage for entrypoint #25

Open AbcSxyZ opened 2 years ago

AbcSxyZ commented 2 years ago

Add test coverage for Dockerfile and the docker-entrypoint.py script.

Run a variety of tests against entrypoint to verify the command which should have been launched by execve. Verifying available files of the image, and the data directory creation.

I hope it can be a good base to test the image, something with meaningful log & understandable enough to be extendable + debuggable.

Need some modification to work appropriately, I will PR it right after:

Launch tests

For now, it's not plugged appropriately with the code and will crash because there is caught error to fix, but for when it will be ok:

Install pip & python packages for the container

apt update && apt install -y python3-pip
python3 -m pip install pytest pytest-testinfra

Add tests files in the container & run pytest in their directory.

Let me know what you think about these tests. And if you have any suggestion of things to tests !

AbcSxyZ commented 2 years ago

All necessary updates are done to make it functional.

Steps to try tests

Install the following packages, for example by adding to the Dockerfile:

RUN apt update && apt install -y python3-pip && \
python3 -m pip install pytest pytest-testinfra

Then add the content of the tests directory inside the container and run pytest, for example:

docker run -it -v $(pwd)/tests:/tests --workdir /tests [image] pytest

You may have some warning, sometime they come, sometime they disappear depending on your configuration. Read/write error of a cache, I didn't investigate further for now.

patricklodder commented 2 years ago

I'm testing with this Dockerfile, created as ./test/Dockerfile.test:

FROM 4b3c1078692b
RUN apt update && apt install -y python3-pip
RUN python3 -m pip install pytest pytest-testinfra
COPY . .
RUN pytest

That works.

AbcSxyZ commented 2 years ago

You may have some warning, sometime they come, sometime they disappear depending on your configuration. Read/write error of a cache, I didn't investigate further for now.

Maybe some issue because of setuid and setgid in entrypoint.py, user change during the call. The first test start as root and finish as dogecoin, all other tests start as dogecoin and finish as dogecoin. Need to handle some rights for the test.

AbcSxyZ commented 2 years ago

I enabled a hook for setgid/setuid. I'm thinking about integration with GA on pull requests.

Do you think we should create a subdirectory (I was thinking of tests/entrypoint), and eventually break all tests from test_dockerfile.py in multiple files ?

To integrate with GA, I was thinking about using a Dockerfile for tests, or to use __init__.py with installation instruction of dependencies, I'm exploring different designs. I will try to find a nice way which enable also caching, it would be great. If you have any idea/suggestion.

patricklodder commented 2 years ago

Do you think we should create a subdirectory (I was thinking of tests/entrypoint), and eventually break all tests from test_dockerfile.py in multiple files ?

I would recommend to create a tests/unit, so that the resulting structure would be:

root
+-- .github/workflows (Automation scripts)
+-- <x>.<y>.<z>/ (one for each Dogecoin Core version)
+-- tests/
|   +-- integration/ (all integration tests under this)
|   +-- unit/ (all unit tests under this)
+-- tools/ (generic tools for test and build processes)
+-- <generic files for the project & product> (license, readme, security, generic manifest)
patricklodder commented 2 years ago

To integrate with GA, I was thinking about using a Dockerfile for tests, or to use init.py with installation instruction of dependencies, I'm exploring different designs.

Ideally, the unit tests for the entrypoint.py can be ran platform independent, outside of the Dockerfile, so that you can just pytest <dir> while you develop locally. Then it's simply a matter of implementation choices. Note that the default ubuntu image on GH Actions ships with python3 so for starters we can just pip install the deps and run it. I also played with the idea of encapsulating the integration test inside a docker container and do docker-within-docker but in the end I thought it better to just keep that execution path simple. Open to other ideas.

I will try to find a nice way which enable also caching, it would be great.

What do you want to cache?

AbcSxyZ commented 2 years ago

The issue I see to do it outside a container is that entrypoint need for now the man pages to retrieve executables options, and will even launch an executable when https://github.com/dogecoin/docker/issues/28 will be implemented.

That's for me the main problem, testable from outside but the host must provide man pages replaced by executables in a near future. Maybe the easiest way is to run it inside a container.

About caching, with my download bandwidth:

$ time apt install -y python3-pip
real    4m59.898s

Annoying between tests if it always needs to be reinstalled.

P.S.: Current tests do not perform unit test of each entrypoint function, but an overall test of the entrypoint behavior through the main. I don't get all subtleties about test naming convention and I'm not able to categorize it as you do, just to be sure you are clear with that.

patricklodder commented 2 years ago

The issue I see to do it outside a container is that entrypoint need for now the man pages to retrieve executables options

Good point, that is a dependency outside of the entrypoint, so in this case we'll want to mock that - basically do something similar to how you mock execution right now. It makes the test of entrypoint.py more generic and not dependent on the Dogecoin Core version used.

Annoying between tests if it always needs to be reinstalled.

Yes. That's why unit tests are better to be executed on the host where you do development. pip3 is also pre-installed on the GA image together with python.

P.S.: Current tests do not perform unit test of each entrypoint function, but an overall test of the entrypoint behavior through the main.

I don't want to go all purist on this, because I think it serves no purpose to do so at this point. If something is covered in unit tests, it's covered. If we miss coverage of some important cases, we can add the test for it. I think we shouldn't worry too much about whether it's called atomically or not right now, maybe something to work on later. In terms of things being tested:

  1. This test validates that entrypoint.py does what it advertises
  2. The build part of the CI validates that there are no errors in the Dockerfile
  3. The integration test part of the CI tests the functionality of the resulting images

That covers all components, except dependencies.

AbcSxyZ commented 2 years ago

To mock man pages/executables, you would like to generate a fake help menu to parse with a limited list of option ? It's an important component to detect which environment variables are valid or not, I'm not sure how we could hook it to reproduce the behavior.

For example, it may be something to test, try one environment variable with 2 different executable with specific argument from each, where the environment variable is only used by one executable, to see if entrypoint doesn't confuse between different help menus.

patricklodder commented 2 years ago

you would like to generate a fake help menu to parse with a limited list of option

let's look inside def executable_options, currently that does 3 things:

  1. read the man file
  2. search it
  3. clean it up

As it is right now, that would need to be mocked completely by a function that just returns a list of options for a given executable. Possible, but that excludes that entire function from testing.

It could be an idea to put the man page reading in its own function and then mock that to just return a couple of fixtures to read instead. Then we can test everything else in that function, including test if the function itself does everything we want it to do and doesn't mess up.

AbcSxyZ commented 2 years ago

Created https://github.com/dogecoin/docker/pull/34 to use executables help menu instead of man pages to get options. To create hook on top of it.

AbcSxyZ commented 2 years ago

Still a bit messy, but it's functional now and can run from the host. I had to add some extra hook and change some behavior. Not friendly to test while it's not in his own folder.

Do we call its folder unit (tests) or entrypoint ? unit isn't really intuitive to me because I do not really see it as unit tests, but if you really want to go for it.

xanimo commented 2 years ago

Still a bit messy, but it's functional now and can run from the host. I had to add some extra hook and change some behavior. Not friendly to test while it's not in his own folder.

Do we call its folder unit (tests) or entrypoint ? unit isn't really intuitive to me because I do not really see it as unit tests, but if you really want to go for it.

so following @patricklodder directions to run these tests as of a day or so ago:

mkdir unit
mv entrypoint_hook.py test_dockerfile.py Dockerfile.test unit/
cd unit
docker build -f Dockerfile.test .

i moved your test files from tests/ to unit/ and created a basic Dockerfile.test identical to @patricklodder's above:

FROM 4c27963a2c8e
RUN apt update && apt install -y python3-pip
RUN python3 -m pip install pytest pytest-testinfra
COPY . .
RUN pytest -v

from the outputted image hash after building 1.14.5/bullseye/Dockerfile:

...
Step 34/34 : CMD ["dogecoind"]
 ---> Using cache
 ---> 4c27963a2c8e
Successfully built 4c27963a2c8e
Successfully tagged xanimo/1.14.5-dogecoin:test-coverage

which resulted in:

docker build -f Dockerfile.test .
Sending build context to Docker daemon  26.11kB
Step 1/5 : FROM 4c27963a2c8e
 ---> 4c27963a2c8e
Step 2/5 : RUN apt update && apt install -y python3-pip
 ---> Using cache
 ---> 09e555c5e61c
Step 3/5 : RUN python3 -m pip install pytest pytest-testinfra
 ---> Using cache
 ---> 05489e6b374c
Step 4/5 : COPY . .
 ---> acb504e88c79
Step 5/5 : RUN pytest --verbose
 ---> Running in 68b3c08af652
============================= test session starts ==============================
platform linux -- Python 3.9.2, pytest-6.2.5, py-1.11.0, pluggy-1.0.0 -- /usr/bin/python3
cachedir: .pytest_cache
rootdir: /dogecoin
plugins: testinfra-6.5.0
collecting ... collected 8 items

test_dockerfile.py::test_entrypoint_executables PASSED                   [ 12%]
test_dockerfile.py::test_environ PASSED                                  [ 25%]
test_dockerfile.py::test_arguments PASSED                                [ 37%]
test_dockerfile.py::test_arguments_double_dash PASSED                    [ 50%]
test_dockerfile.py::test_mixing_argument_and_env PASSED                  [ 62%]
test_dockerfile.py::test_equal_argv_and_env PASSED                       [ 75%]
test_dockerfile.py::test_help_debug PASSED                               [ 87%]
test_dockerfile.py::test_datadir[local] PASSED                           [100%]

============================== 8 passed in 0.09s ===============================
Removing intermediate container 68b3c08af652
 ---> 5bbfd0934846
Successfully built 5bbfd0934846

:fireworks: :partying_face: woohoo!

so with that said it's almost there. am wondering how we're going to 'plug' this in? will it be ran as part of CI? not sure the grand vision but we could potentially use ONBUILD within our existing Dockerfile (at the bottom) to pass the image hash as it executes after our main Dockerfile is built and run tests and then delete/clean up after execution? outside of that i'd say a squash + rebase is needed and then ACK from me. :+1:

i also pushed this as reference in case needed but in no way is 'the correct' way, just how i understood testing this PR.

AbcSxyZ commented 2 years ago

I will add some fixes about global variables, and try to enable only hooks partially when tests are launched inside containers, to be closer to the real behavior.

patricklodder commented 2 years ago

try to enable only hooks partially when tests are launched inside containers, to be closer to the real behavior.

Recommend against that, it will make tests non-deterministic.

I can propose an integration test for the chmod results for you if you want.

AbcSxyZ commented 2 years ago

I will do something in a separate commit, it looks interesting to me to just add a way to disable some hook of EntrypointHook, just to see what it can look like.

Having 2 mode may enable us to tests some functions when inside the container, like entrypoint.get_help.

patricklodder commented 2 years ago

Not inside unit tests, I coded us an integration test framework for that.

AbcSxyZ commented 2 years ago

Some fun with pylint:

************* Module test_dockerfile
test_dockerfile.py:10:24: E1101: Module 'pytest' has no 'executables_folder' member (no-member)
test_dockerfile.py:19:20: E1101: Module 'pytest' has no 'datadir' member (no-member)
test_dockerfile.py:20:17: E1101: Module 'pytest' has no 'user' member (no-member)
test_dockerfile.py:21:17: E1101: Module 'pytest' has no 'PATH' member (no-member)
test_dockerfile.py:25:17: E1101: Module 'pytest' has no 'user' member (no-member)
test_dockerfile.py:26:17: E1101: Module 'pytest' has no 'PATH' member (no-member)
test_dockerfile.py:34:24: E1101: Module 'pytest' has no 'datadir' member (no-member)
test_dockerfile.py:45:24: E1101: Module 'pytest' has no 'datadir' member (no-member)
[lol]
[...]
************* Module conftest
conftest.py:19:23: R1732: Consider using 'with' for resource-allocating operations (consider-using-with) [How do I get a file for the entire test ?..]

Pylint does have some trouble to work with pytest, it raises some false positive.

There is some alternative python linter for pytest like pytest-pylint (maybe not well maintained...), I saw a Stack Overflow comment suggesting the following:

  • use another unittest framework, less dynamic than py.test
  • silence the warnings / errors on your test code manually
  • use another linter which is happier than pylint on py.test (I'm interested to know how pychecker / pyflakes fare on that code)
  • write the astng plugin which will help astng grok the pylib tricks and submit it as a patch to the astng maintainers (and get extra credit from that)

The solution I chose for now is to hide those errors, thought it could be acceptable here, for these specifics errors:

[entrypoint-tests]
disable = consider-using-with,
    no-member

Pylint need some adjustment in any case, CI tests crashes without pytest installed, see GA results.

patricklodder commented 2 years ago

Pylint does have some trouble to work with pytest, it raises some false positive.

Ideally, our pylint check should not lint pytest, but limit scope only our code. I'm trying to figure out what happens if we inline ignore the pytest import and if that still yields meaningful results, or, import pytest with the linting run and then exclude that module's errors rather than the ones inherited under the module we wrote... but ignores are needed.

Once we introduce ignores, it is probably prudent to explicitly list the allowed linter ignores and document why they are allowed.

AbcSxyZ commented 2 years ago

@xanimo am wondering how we're going to 'plug' this in? will it be ran as part of CI?

Yes, more tests to automate :)

Ready for a new round of review. Recent changes allow tests to run both from host and inside a container. I implemented 2 modes for tests (introduced by https://github.com/dogecoin/docker/pull/25/commits/174640a0ab2824f0efcc02d1c43a43adc2bd8c34), it detects if it runs inside a container to avoid the hook of entrypoint.get_help, to get more in real conditions tests of the entrypoint. Final CI tests would run inside a container to be more strict.

May enable some specifics tests to runs inside a container which is not permitted by, for example verify the behavior of entrypoint for non-dogecoin commands.

Pass pylint tests with exception from pylintrc.

PS: Some files may need a renaming (any suggestions ?). test_dockerfile.py for example. The tree structure should probably be more organized, tests factorized. Maybe we should think the relation with integration tests to facilitate the launch of both tests, providing a single interface.

xanimo commented 2 years ago

@xanimo am wondering how we're going to 'plug' this in? will it be ran as part of CI?

Yes, more tests to automate :)

Ready for a new round of review. Recent changes allow tests to run both from host and inside a container. I implemented 2 modes for tests (introduced by 174640a), it detects if it runs inside a container to avoid the hook of entrypoint.get_help, to get more in real conditions tests of the entrypoint. Final CI tests would run inside a container to be more strict.

May enable some specifics tests to runs inside a container which is not permitted by, for example verify the behavior of entrypoint for non-dogecoin commands.

Pass pylint tests with exception from pylintrc.

PS: Some files may need a renaming (any suggestions ?). test_dockerfile.py for example. The tree structure should probably be more organized, tests factorized. Maybe we should think the relation with integration tests to facilitate the launch of both tests, providing a single interface.

awesome! will dig into this later this afternoon when i can checkout your changes and address your comments/concerns properly, have to run some errands rq. :heart:

AbcSxyZ commented 2 years ago

New changes related to previous comments. I will need to update comments, at least for pylint but I want to reread it globally. I will do it later today or tomorrow.

For now, functionally I think it's good, I organized the tree structure (pylintrc to move away probably), dividing all tests in separate files, creating a hook directory for tools needed. I also enabled some default environment variables (PATH & USER) in EntrypointHook to avoid repetition in tests.

It gives the following structure:

.
├── hooks
│   ├── command.py
│   ├── entrypoint_hook.py
│   └── help_menus.py
├── conftest.py
├── test_arguments.py
├── test_datadir.py
├── test_environment_hyphen.py
├── test_environment.py
├── test_executables.py
└── test_mix_argument_environment.py

Possibly not far from being ready :)

Recap on how to launch tests

It can run from hosts or from container, with fewer hooks to enable (entrypoint.get_help avoided) in containers.

From host

Move into the unit directory where tests are. You need to copy the entrypoint.py from 1.14.5/bulleseye in the unit dir. Run python3 -m pip install pytest pytest-testinfra & launch pytest command.

From a container

Build image from 1.14.5/bullseye. Mount unit test files into a running container and run tests, for example:

# Inside the host
docker run -it -v $(pwd)/unit:/tests [image-tag] bash

# From the container bash
apt update && apt install -y python3-pip
python3 -m pip install pytest

# Then move in mounted dir and run tests
cd /tests && pytest
AbcSxyZ commented 2 years ago

I moved tests about datadir creation into CI (see https://github.com/dogecoin/docker/issues/50) + I added some tests for https://github.com/dogecoin/docker/pull/34 to test if -help-debug options are converted properly into environment variables.

AbcSxyZ commented 2 years ago

Integration to GitHub Action added, you can see directly results from PR CI tests :)

 ============================= test session starts ==============================
platform linux -- Python 3.9.2, pytest-6.2.5, py-1.11.0, pluggy-1.0.0 -- /usr/bin/python3
cachedir: .pytest_cache
rootdir: /tests
collecting ... collected 9 items

test_arguments.py::test_arguments PASSED                                 [ 11%]
test_arguments.py::test_arguments_double_dash PASSED                     [ 22%]
test_environment.py::test_environment PASSED                             [ 33%]
test_environment_hyphen.py::test_environment_hyphen PASSED               [ 44%]
test_executables.py::test_entrypoint_executables PASSED                  [ 55%]
test_extended_options.py::test_extended_options PASSED                   [ 66%]
test_extended_options.py::test_invalid_extended PASSED                   [ 77%]
test_mix_argument_environment.py::test_mixing_argument_and_env PASSED    [ 88%]
test_mix_argument_environment.py::test_equal_argv_and_env PASSED         [100%]

============================== 9 passed in 0.13s ===============================

Squash to clean up the PR. Need to configure linter to handle issues, to do in a separate PR prior to it.

EDIT: pylitnrc removed in favor of https://github.com/dogecoin/docker/pull/52

AbcSxyZ commented 2 years ago

I removed the container mode.

About sorting, I removed it. But I wasn't sure actually how it was managed by entrypoint.py. Environment variables, when given by docker run as -e options, do not preserve an order (I don't even know how shell manage it internally, perhaps it's the responsible).

Completing https://github.com/dogecoin/docker/issues/46, the current behavior is:

[environment variables sorted by help menu appearance] [argv]

As you can see in entrypoint.convert_env: https://github.com/dogecoin/docker/blob/9dcc1697ea6c94992192b3079d53f28d4ebe4130/1.14.5/bullseye/entrypoint.py#L109

It finds all options through help menu, iterate through those options and try to find if it's available in the environment. I fixed tests in https://github.com/dogecoin/docker/pull/25/commits/1105189556980cfa1840c46e51dcc7a7bea022d4 to reflect this order.

Merry Christmas :santa: