Skip to content

Tests: Add system tests for groupadd in test_groupadd.py#1655

Merged
ikerexxe merged 3 commits into
shadow-maint:masterfrom
asakure:system_tests_groupadd
Jul 1, 2026
Merged

Tests: Add system tests for groupadd in test_groupadd.py#1655
ikerexxe merged 3 commits into
shadow-maint:masterfrom
asakure:system_tests_groupadd

Conversation

@asakure

@asakure asakure commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Add system tests for groupadd in test_groupadd.py.

@ikerexxe ikerexxe left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Apart from the inline comments I have some generic ones:

  • Add the path to the test that you are transforming in the commit message. Just to give you an example as how it should look like: b19a9d2 and 89aaea2
  • Use and instead of the informal & in test steps

Comment thread tests/system/tests/test_groupadd.py Outdated
Comment thread tests/system/tests/test_groupadd.py Outdated
Comment thread tests/system/tests/test_groupadd.py Outdated
Comment thread tests/system/tests/test_groupadd.py Outdated
Comment thread tests/system/tests/test_groupadd.py Outdated
Comment thread tests/system/tests/test_groupadd.py Outdated
Comment thread tests/system/tests/test_groupadd.py Outdated
Comment thread tests/system/tests/test_groupadd.py Outdated
Comment thread tests/system/tests/test_groupadd.py Outdated
Comment thread tests/system/tests/test_groupadd.py Outdated
@asakure asakure force-pushed the system_tests_groupadd branch from 8ba884c to 9cafdaf Compare June 24, 2026 18:38

@ikerexxe ikerexxe left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This has been improved quite a bit. There are still some minor things to improve, but I think we are on the right track

Comment thread tests/system/tests/test_groupadd.py Outdated
Comment thread tests/system/tests/test_groupadd.py Outdated
Comment thread tests/system/tests/test_groupadd.py
Comment thread tests/system/tests/test_groupadd.py Outdated
Comment thread tests/system/tests/test_groupadd.py
Comment thread tests/system/tests/test_groupadd.py Outdated
Comment thread tests/system/tests/test_groupadd.py Outdated
@ikerexxe

Copy link
Copy Markdown
Collaborator

Please also check CI failures

@asakure asakure force-pushed the system_tests_groupadd branch 2 times, most recently from 2680fa4 to 4010eba Compare June 25, 2026 10:42

@ikerexxe ikerexxe left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

In addition to the inline comments, you should run the linters:

flake8 .
pycodestyle .
isort --check-only .
black --check --diff .
mypy --install-types --non-interactive tests

They will raise a set of improvements that will make your tests run without problems (i.e. missing imports)

Comment thread tests/system/tests/test_groupadd.py Outdated
Comment thread tests/system/tests/test_groupadd.py Outdated
Comment thread tests/system/tests/test_groupadd.py Outdated
Comment thread tests/system/tests/test_groupadd.py Outdated
Comment thread tests/system/tests/test_groupadd.py Outdated
This is Python transformation of the test located in
`tests/grouptools/groupadd/05_groupadd_set_GID/groupadd.test`
which checks that `groupadd` can create group with sepcified GID
using -g flag

Signed-off-by: Akshay Sakure <asakure@redhat.com>
@asakure asakure force-pushed the system_tests_groupadd branch 2 times, most recently from b175b1f to 4b71f89 Compare June 30, 2026 14:13
@asakure

asakure commented Jun 30, 2026

Copy link
Copy Markdown
Contributor Author

Hi @ikerexxe I have made suggested changes & ran the linters as well to confirm if there is any improvement scope, let me know if looks better now.

@ikerexxe ikerexxe left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Almost there, just two minor things to improve and we are done

Comment thread tests/system/tests/test_groupadd.py Outdated
Comment thread tests/system/tests/test_groupadd.py Outdated
This is Python transformation of the test located in
'tests/grouptools/groupadd/04_groupadd_set_password/groupadd.test'
which checks that 'groupadd' can create group with password using
-p flag

Signed-off-by: Akshay Sakure <asakure@redhat.com>
@asakure asakure force-pushed the system_tests_groupadd branch 2 times, most recently from d9dc7fc to 125cdf9 Compare July 1, 2026 09:55
@ikerexxe

ikerexxe commented Jul 1, 2026

Copy link
Copy Markdown
Collaborator

One last thing to get CI green and merge this PR:

Run source .venv/bin/activate && isort --check-only .
ERROR: /home/runner/work/shadow/shadow/tests/system/tests/test_groupadd.py Imports are incorrectly sorted and/or formatted.
Skipped 1 files

@asakure asakure force-pushed the system_tests_groupadd branch from 125cdf9 to 2845d9d Compare July 1, 2026 12:13
This is Python transformation of the test located in
'tests/grouptools/groupadd/06_groupadd_-f_add_existing_group/groupadd.test'
which checks that 'groupadd' completes successfully while force-creating
an existing group using -f flag

Signed-off-by: Akshay Sakure <asakure@redhat.com>
@asakure asakure force-pushed the system_tests_groupadd branch from 2845d9d to c2a9d70 Compare July 1, 2026 12:17

@ikerexxe ikerexxe left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM!

openSUSE infrastructure seems to be down and CI can't install the dependencies. I'm merging anyways since all other runners succeeded

@ikerexxe ikerexxe merged commit a67b830 into shadow-maint:master Jul 1, 2026
34 of 41 checks passed
@asakure

asakure commented Jul 1, 2026

Copy link
Copy Markdown
Contributor Author

openSUSE infrastructure seems to be down and CI can't install the dependencies. I'm merging anyways since all other runners succeeded

I see, I was wondering why CI is still failing even after making the changes. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants