Skip to content

removing netgroup intg test#8356

Merged
spoore1 merged 1 commit intoSSSD:masterfrom
danlavu:tests-rm-netgroups
Jan 15, 2026
Merged

removing netgroup intg test#8356
spoore1 merged 1 commit intoSSSD:masterfrom
danlavu:tests-rm-netgroups

Conversation

@danlavu
Copy link

@danlavu danlavu commented Jan 15, 2026

No description provided.

@danlavu danlavu added Waiting for review Tests Trivial A single reviewer is sufficient to review the Pull Request backport-to-sssd-2-11 backport-to-sssd-2-12 labels Jan 15, 2026
@danlavu
Copy link
Author

danlavu commented Jan 15, 2026

The intg tests have been rewritten by @madhuriupadhye, #8272 and #8262

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request removes the test_netgroup.py integration test suite. This is a significant change that lacks any explanation in the PR description. Removing a large number of tests without justification is highly discouraged as it reduces test coverage and increases the risk of future regressions. It is critical to clarify if these tests are obsolete or are being replaced by another test suite. Additionally, the cleanup is incomplete, leaving behind orphaned files like sss_netgroup_thread_test.c and sssd_netgroup.py, along with their associated build configurations. These should be removed to avoid accumulating dead code.

I am having trouble creating individual review comments. Click here to see my feedback.

src/tests/intg/test_netgroup.py (1-539)

critical

This pull request removes a large suite of integration tests for netgroup functionality without any explanation. Deleting tests is highly risky as it removes a safety net against future regressions. Please provide a clear justification for this removal in the pull request description. If these tests are being superseded by others (e.g., the ones in src/tests/multihost/alltests/test_netgroup.py), this should be stated.

src/tests/intg/test_netgroup.py (1-539)

high

The removal of this test file leaves several orphaned components, which creates dead code and increases maintenance burden.

  • The test test_innetgr_with_threads (lines 537-539) was the sole user of the sss_netgroup_thread_test binary. The C source file sss_netgroup_thread_test.c and its build rules in src/tests/intg/Makefile.am are now obsolete.
  • The helper module sssd_netgroup.py (imported on line 35) was used extensively here and is likely now unused.

Please remove these orphaned files and build configurations as part of this pull request.

Copy link
Contributor

@spoore1 spoore1 left a comment

Choose a reason for hiding this comment

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

Looks pretty trivial and straight forward.

LGTM.

Reviewed-by: Scott Poore <spoore@redhat.com>
@sssd-bot
Copy link
Contributor

The pull request was accepted by @spoore1 with the following PR CI status:


🟢 CodeFactor (success)
🟢 CodeQL (success)
🟡 rpm-build:centos-stream-10-x86_64:upstream (in_progress)
🟡 rpm-build:fedora-42-x86_64:upstream (in_progress)
🟡 rpm-build:fedora-43-x86_64:upstream (in_progress)
🟡 rpm-build:fedora-rawhide-x86_64:upstream (in_progress)
🟢 Analyze (target) / cppcheck (success)
🟢 Build / freebsd (success)
🟡 Build / make-distcheck (in_progress)
🟡 ci / intgcheck (centos-10) (in_progress)
🟡 ci / intgcheck (fedora-42) (in_progress)
🟡 ci / intgcheck (fedora-43) (in_progress)
🟡 ci / intgcheck (fedora-44) (in_progress)
🟢 ci / prepare (success)
🟡 ci / system (centos-10) (in_progress)
🟡 ci / system (fedora-42) (in_progress)
🟡 ci / system (fedora-43) (in_progress)
🟡 ci / system (fedora-44) (in_progress)
➖ Coverity scan / coverity (skipped)
🟢 Static code analysis / codeql (success)
🟢 Static code analysis / pre-commit (success)
🟢 Static code analysis / python-system-tests (success)


There are unsuccessful or unfinished checks. Make sure that the failures are not related to this pull request before merging.

@spoore1 spoore1 merged commit ab7a7f4 into SSSD:master Jan 15, 2026
23 of 25 checks passed
@sssd-bot
Copy link
Contributor

The pull request was accepted by @spoore1 with the following PR CI status:


🟢 CodeFactor (success)
🟢 CodeQL (success)
🟡 osh-diff-scan:fedora-rawhide-x86_64:upstream (in_progress)
🟢 rpm-build:centos-stream-10-x86_64:upstream (success)
🟢 rpm-build:fedora-42-x86_64:upstream (success)
🟢 rpm-build:fedora-43-x86_64:upstream (success)
🟢 rpm-build:fedora-rawhide-x86_64:upstream (success)
🟢 Analyze (target) / cppcheck (success)
➖ Build / freebsd (skipped)
➖ Build / make-distcheck (skipped)
➖ ci / intgcheck (skipped)
➖ ci / prepare (skipped)
➖ ci / system (skipped)
➖ Coverity scan / coverity (skipped)
🟢 Static code analysis / codeql (success)
🟢 Static code analysis / pre-commit (success)
🟢 Static code analysis / python-system-tests (success)


There are unsuccessful or unfinished checks. Make sure that the failures are not related to this pull request before merging.

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

Labels

Accepted backport-to-sssd-2-11 backport-to-sssd-2-12 Tests Trivial A single reviewer is sufficient to review the Pull Request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants