Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add test coverage for entrypoint #25

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Add test coverage for entrypoint #25

wants to merge 4 commits into from

Conversation

AbcSxyZ
Copy link
Contributor

@AbcSxyZ AbcSxyZ commented Dec 4, 2021

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 AbcSxyZ marked this pull request as draft December 4, 2021 15:25
@patricklodder patricklodder added the qa Such quality label Dec 4, 2021
@patricklodder patricklodder mentioned this pull request Dec 4, 2021
@AbcSxyZ AbcSxyZ mentioned this pull request Dec 4, 2021
@AbcSxyZ AbcSxyZ marked this pull request as ready for review December 6, 2021 16:51
@AbcSxyZ
Copy link
Contributor Author

AbcSxyZ commented Dec 6, 2021

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 patricklodder self-requested a review December 6, 2021 17:47
@patricklodder
Copy link
Member

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
Copy link
Contributor Author

AbcSxyZ commented Dec 8, 2021

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 AbcSxyZ marked this pull request as draft December 10, 2021 13:59
@AbcSxyZ
Copy link
Contributor Author

AbcSxyZ commented Dec 10, 2021

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
Copy link
Member

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
Copy link
Member

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?

Copy link
Member

@patricklodder patricklodder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initial review.

Overall looks really good, but some things should be moved from unit to integration because they don't test atomic functions, but end results external to entrypoint, and some uses of os probably will prevent portability of the tests.

I think if we fix these, this also becomes platform independent, so that a windows dev can run these tests natively too.

tests/entrypoint_hook.py Outdated Show resolved Hide resolved
tests/test_dockerfile.py Outdated Show resolved Hide resolved
hook.test(test_args, test_environ, result_args, result_environ)
assert hook.result == hook.reference, hook.error_msg()

#Test datadir metadata
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This must be moved to an integration test too

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed it, keep record of the test in #33 to convert it in CI tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made a mistake in this comment, I didn't remove this test, it's the previous one. It verifies that datadir is properly created by the entrypoint + chown, it manages the creation of the directory. Don't you think I should keep it here ?

hook.test(test_args, test_env, result_args, result_env)
assert hook.result == hook.reference, hook.error_msg()

def test_equal_argv_and_env(hook):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a question: why is this designed like this?

I'm not entirely sure about the feature. I'd like to know why we do that because I think it can create error situations: if I do -e PAYTXFEE=1 and add -paytxfee=0.1, which one will be used by dogecoind and how do I know that as an operator. Shouldn't we protect the operator against that?

Copy link
Contributor Author

@AbcSxyZ AbcSxyZ Dec 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had no idea what should I do in this case, just had the idea to test it. The user still can pass multiple times the same arguments, Dogecoin Core accept it, with or without this environment.

I don't know what happen internally, but the following start the node:

./bin/dogecoind -maxtxfee=1 -maxtxfee=2

Idk if it should be handled here, or maybe improve dogecoin/dogecoin#1313 to let Dogecoin Core deal with it ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dogecoind takes the last entry. Which in this case means the last arg overwrites the env value.

So if we're at this point not protecting the user (I'm fine with this, just need to make a decision on it) perhaps it makes sense to do a different value assignment for an env and arg of the same type and then test if the resulting order is correct. A rewrite in the logic could easily change this and mess things up, so this would be really good to test.

We can let dogecoin/dogecoin#1313 handle this better for users regardless of how they deploy, yes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sense to do a different value assignment for an env and arg of the same type and then test if the resulting order is correct

You're speaking about the second assertion of this test ?

Copy link
Contributor Author

@AbcSxyZ AbcSxyZ Dec 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can be great to protect this behavior, but yeah maybe it's more appropriate to handle double configuration inside the Core, make sense to me. Needed just for args for example, and would duplicate verification if it's done here.


def abs_path(executable):
"""Build manually (expected) executable absolute path"""
return os.path.join(EXECUTABLES_FOLDER, executable)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would add a \ when ran on windows, I think that's not what we're looking for here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Depending on what we want to do, maybe. The answer may differ if we can run those tests from outside a container.

test_environ = {
"DATADIR" : "/dogecoin/.dogecoin",
"USER" : "dogecoin",
"PATH" : os.environ["PATH"],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure about os.environ - can we do this with a fixture instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same problem. If shared with the host, yes it may be an issue, if it's shared by the container it shouldn't be a concern.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me ask it differently: what benefit does this give over a static fixture? Do we need this to be a working path? If yes, for what?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The entrypoint use PATH to resolve executable location in shutil.which:

executable_path = shutil.which(executable)

Entrypoint need at least /usr/local/bin in PATH to work properly [in a container].

@AbcSxyZ
Copy link
Contributor Author

AbcSxyZ commented Dec 10, 2021

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 #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
Copy link
Member

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
Copy link
Contributor Author

AbcSxyZ commented Dec 10, 2021

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
Copy link
Member

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
Copy link
Contributor Author

AbcSxyZ commented Dec 10, 2021

Created #34 to use executables help menu instead of man pages to get options. To create hook on top of it.

@patricklodder patricklodder added this to ⛔️ Blocked in Review board Dec 11, 2021
@patricklodder patricklodder moved this from ⛔️ Blocked to 🚀 Everything else in Review board Dec 11, 2021
@AbcSxyZ
Copy link
Contributor Author

AbcSxyZ commented Dec 12, 2021

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.

@AbcSxyZ AbcSxyZ marked this pull request as ready for review December 12, 2021 01:29
#Disable setgid & setuid behavior
os.setgid = lambda _ : None
os.setuid = lambda _ : None
shutil.which = lambda executable : f"/usr/local/bin/{executable}"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move line and/or update doc

Comment on lines 7 to 15
EXECUTABLES_FOLDER = "/usr/local/bin/"

#Set tests user and directories
TEST_USER = os.environ.get("TEST_USER", None)
if TEST_USER is None:
TEST_USER = "dogecoin"

TEST_DIRECTORY = tempfile.TemporaryDirectory()
TEST_DATADIR = TEST_DIRECTORY.name
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe some possibilities do define tests variables in conftest.py.

@xanimo
Copy link
Member

xanimo commented Dec 12, 2021

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

🎆 🥳 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. 👍

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 AbcSxyZ marked this pull request as draft December 12, 2021 14:50
@AbcSxyZ
Copy link
Contributor Author

AbcSxyZ commented Dec 12, 2021

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
Copy link
Member

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
Copy link
Contributor Author

AbcSxyZ commented Dec 12, 2021

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
Copy link
Member

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

@AbcSxyZ
Copy link
Contributor Author

AbcSxyZ commented Dec 12, 2021

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
Copy link
Member

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 AbcSxyZ marked this pull request as ready for review December 12, 2021 20:11
@AbcSxyZ
Copy link
Contributor Author

AbcSxyZ commented Dec 12, 2021

@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.

@xanimo
Copy link
Member

xanimo commented Dec 12, 2021

@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. ❤️

@AbcSxyZ AbcSxyZ marked this pull request as draft December 14, 2021 14:10
@AbcSxyZ
Copy link
Contributor Author

AbcSxyZ commented Dec 14, 2021

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 AbcSxyZ marked this pull request as ready for review December 14, 2021 14:35
@AbcSxyZ AbcSxyZ changed the title Add test coverage for Dockerfile Add test coverage for entrypoint Dec 15, 2021
@AbcSxyZ
Copy link
Contributor Author

AbcSxyZ commented Dec 17, 2021

I moved tests about datadir creation into CI (see #50) + I added some tests for #34 to test if -help-debug options are converted properly into environment variables.

@AbcSxyZ
Copy link
Contributor Author

AbcSxyZ commented Dec 17, 2021

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 #52

@patricklodder patricklodder moved this from 🚀 Everything else to ⛔️ Blocked in Review board Dec 18, 2021
@AbcSxyZ AbcSxyZ closed this Dec 19, 2021
@AbcSxyZ AbcSxyZ reopened this Dec 19, 2021
@patricklodder patricklodder moved this from ⛔️ Blocked to 🚀 Everything else in Review board Dec 19, 2021
Copy link
Member

@patricklodder patricklodder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First off, much of this is really good ❤️

I have 2 blockers:

  1. Please get rid of containerization detection. Any test goals that are achieved with this MUST be reflected in integration tests.
  2. The sort of resulting arguments in a command must go, it prevents testing order of argument parsing, which matters.

tests/unit/hooks/command.py Outdated Show resolved Hide resolved
tests/unit/hooks/entrypoint_hook.py Outdated Show resolved Hide resolved
tests/unit/hooks/entrypoint_hook.py Outdated Show resolved Hide resolved
.github/workflows/build-ci.yml Show resolved Hide resolved
tests/unit/test_mix_argument_environment.py Outdated Show resolved Hide resolved
@AbcSxyZ
Copy link
Contributor Author

AbcSxyZ commented Dec 24, 2021

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 #46, the current behavior is:

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

As you can see in entrypoint.convert_env:

for option in man_options:

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 1105189 to reflect this order.

Merry Christmas 🎅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
qa Such quality
Projects
Review board
🚀 Everything else
Development

Successfully merging this pull request may close these issues.

None yet

3 participants