-
Notifications
You must be signed in to change notification settings - Fork 240
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
System test framework #1131
base: master
Are you sure you want to change the base?
System test framework #1131
Conversation
@hallyn I'd appreciate your feedback on this PR, particularly the tests in the |
Thanks for the ping, I'm putting this on my next_actions list. |
9460da1
to
a454e51
Compare
For the moment, let's just ignore the CI errors. I'll fix them when I figure out how to do it |
How should I run the tests in my computer? Also, are those tests (potentially) destructive? That is, do I need to run them inside a container? Or does the test suite spin up containers as necessary? |
log_level=ProcessLogLevel.Error, | ||
) | ||
|
||
return PurePosixPath(result.stdout_lines[-1].strip()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, but the script does set -e, so result.stdout_lines could in theory be empty?
I guess it'll just give us an indexerror so no big deal? However, catching a return value != 0 early might be less confusing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about the following?
if not result.stdout_lines:
return PurePosixPath(result.stdout_lines[-1].strip())
else:
return -1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It shouldn't be empty, if the script fails it raises ProcessError
from run
since raise_on_error=True
by default.
I'm only partly through this (at patch 5), but it looks good so far, thanks. |
The instructions from You can of course run the test on you own system, but I'd recommend against it since you may break your system with a shadow change. I plan to extend the documentation to explain how the framework works, the API, how to integrate it, etc. But that will come in another PR, once we all agree that the PoC is in a good shape and it can be merged.
So you've already gone through the first test cases. What do you think about them? Is the API clear to manage shadow and other utilities clear enough? |
a454e51
to
9bdea5e
Compare
Just to check if I've understood that: So, if I run |
afc616c
to
68d5201
Compare
Yes, the test suite will run within |
Ok, now the CI is in the state where I wanted it to be last week. Alpine build is failing due to https://gitlab.alpinelinux.org/alpine/aports/-/issues/16633. This is already fixed but it takes some time for those changes to arrive to users. |
68d5201
to
7e672dc
Compare
To make the code more portable to distributions that don't have bash in /usr/bin. Resolves: next-actions#95 Link: <shadow-maint/shadow#1131> Reported-by: Iker Pedrosa <ipedrosa@redhat.com> Reported-by: Alejandro Colomar <alx@kernel.org> Suggested-by: Alejandro Colomar <alx@kernel.org>
To make the code more portable to distributions that don't have bash in /usr/bin. Resolves: #95 Link: <shadow-maint/shadow#1131> Reported-by: Iker Pedrosa <ipedrosa@redhat.com> Reported-by: Alejandro Colomar <alx@kernel.org> Suggested-by: Alejandro Colomar <alx@kernel.org>
7610fa4
to
3baa96e
Compare
I think so, yes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, I went over the code briefly.
I think this needs clean up. It looks like you copy&paste lost of stuff from sssd-test-framework that you actually don't use. I suggest you to go over it and remove what is unused (like SSHKillableProcess, BaseTopologyController, ShadowTopologyController) to simplify the code.
Some things, maybe, you do not need to copy&paste but import it instead (like stuff in tools.py). But that is of course up to your preference.
I'd prefer to avoid any dependency towards a project with no direct relation to this one (i.e. sssd-test-framework). Shadow's test framework should be smaller and way simpler, even simpler than I expected from your last comments. |
3baa96e
to
ae90094
Compare
As discussed at length, this is the implementation of the new system tests framework for shadow. This is a proof of concept that contains the key elements to be able to run basic user (i.e. useradd, usermod) and group (i.e. usermod) tests. If you like the framework the rest of the functionality will be added in the future. Some useful facts: * It is implemented in python * It is based on pytest and pytest-mh * It works on all the distributions that are part of our CI * It can be run in the cloud (VM or container) as well as on-premises * After the execution of each test the environment is cleaned up * Logs and other artifacts for failed tests are collected * It has a rich API that can be extended and extended to cover new functionalities Closes: shadow-maint#835 Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
This is the transformation to Python of the test located in `tests/usertools/01/01_useradd_add_user.test`, which checks that `useradd` is able to create a new user and its corresponding group and home folder. Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
This is the transformation to Python of the test located in `tests/usertools/01/02_useradd_recreate_deleted_user.test`, which checks that `useradd` is able to create again a removed user. Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
This is the transformation to Python of the test located in `tests/usertools/01/10_usermod_rename_user.test`, which checks that `usermod` is able to rename a user. The test checks that the new user, the group and home folder exists. Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
This is the transformation to Python of the test located in `tests/usertools/01/18_userdel_remove_homedir.test`, which checks that `userdel` is able to delete a user and its homedir. The test checks that the user, the group and the home folder don't exist. Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
This is the transformation to Python of the test located in `tests/grouptools/groupadd/02_groupadd_add_group_GID_MIN/groupadd.test`, which checks that `groupadd` is able to create a new group. Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
This is the transformation to Python of the test located in `tests/grouptools/groupmod/01_groupmod_change_gid/groupmod.test`, which checks that `groupmod` is able to change the GID of a group. Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
This is the transformation to Python of the test located in `tests/grouptools/groupdel/01_groupdel_delete_group/groupdel.test`, which checks that `groupdel` is able to delete a group. Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
In order to have consistent behaviour among all distributions, the same configuration needs to be shared. That is why we are going to use the `etc/login.defs` file and enable CREATE_HOME so that the home dir is created automatically. This is not the default configuration used in all distributions, but it is the most common one. Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
Run the newly created system tests in CI and collect artifacts. Signed-off-by: Iker Pedrosa <ipedrosa@redhat.com>
ae90094
to
7aa7a50
Compare
@alejandro-colomar @hallyn it'd be nice to have a final review from your side |
Mine will be more an opinion than an actual review: I don't understand Python. I can maybe read 5 or 10 lines for a small script, but this is huge, and inscrutable to me. I wish this was done in bash(1), which is a language I can read much better (and I guess many Linux/Unix programmers too, but I don't know). However, the old framework was written as shell scripts, and I only used it maybe once or twice, so I can't complain. So if you and Serge understand this framework and make it easy for others to use, it's a net improvement. I'll use it, but won't be able to contribute to it. Thank you for your work! I appreciate it, even if I don't understand it. :-) Anyway, if I ever have the time to write a testing framework in bash(1), I'll let you know. For the time being, this is better than no tests (nobody was adding tests with the old test suite). I have other food in my plate for the time being. :-) |
As discussed at length, this is the implementation of the new system
tests framework for shadow. This is a proof of concept that contains the
key elements to be able to run basic user (i.e. useradd, usermod) and
group (i.e. usermod) tests. If you like the framework the rest of the
functionality will be added in the future.
Some useful facts:
functionalities
In addition, the PR contains a set of tests that make use of the framework
to test the functionality. These tests are located under
tests/system/tests
.They offer a good example of the high-level API and how to model the test
cases.
Finally, the system test are run in the CI. This way we make sure that the
code can run successfully and avoid any regressions.
Closes: #835