Skip to content

Conversation

@danlavu
Copy link

@danlavu danlavu commented Oct 31, 2025

No description provided.

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 old integration tests for infopipe and makes several beneficial updates to the newer system tests. The removal of src/tests/intg/test_infopipe.py and its corresponding entry in Makefile.am is correct. The modifications in src/tests/system/tests/test_infopipe.py improve the test suite by refactoring assertions into loops and using pytest.parametrize to create more comprehensive and maintainable tests. The changes to topology markers to focus on LDAP seem intentional and align with the goal of refining the test scope. Overall, the changes are positive and I did not find any high or critical severity issues.

@danlavu
Copy link
Author

danlavu commented Oct 31, 2025

A lot of the comments are in the jira ticket. A lot of these test cases are being dropped. All of the negative test cases. Right now in it's current the state, the ifp framework class does not handle invalid values without throwing several exceptions which will cause the test cases to exit when it fails. Even though that is the expected behavior. Please look at the JIra issue for more detail. The rest of the coverage IMO is adequate, if anybody thinks otherwise, we can re-evaluate them together.

Copy link
Contributor

@jakub-vavra-cz jakub-vavra-cz left a comment

Choose a reason for hiding this comment

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

See inline comment otherwise LGTM.

@danlavu danlavu force-pushed the tests-rm-intg-infopipe branch from 8695f9b to 0cb3eca Compare November 10, 2025 21:01
Copy link
Contributor

@jakub-vavra-cz jakub-vavra-cz left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@aplopez aplopez left a comment

Choose a reason for hiding this comment

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

LGTM

@sssd-bot
Copy link

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


🟢 CodeFactor (success)
🟢 CodeQL (success)
NEUTRAL osh-diff-scan:fedora-rawhide-x86_64:upstream (neutral)
🟢 rpm-build:centos-stream-10-x86_64:upstream (success)
🟢 rpm-build:fedora-41-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) / All tests are successful (success)
🟢 Analyze (target) / cppcheck (success)
🟢 Build / freebsd (success)
🟢 Build / make-distcheck (success)
🔴 ci / All tests are successful (failure)
🟢 ci / intgcheck (centos-10) (success)
🔴 ci / intgcheck (fedora-41) (failure)
🟢 ci / intgcheck (fedora-42) (success)
🔴 ci / intgcheck (fedora-43) (failure)
🟢 ci / intgcheck (fedora-44) (success)
🟢 ci / prepare (success)
🟢 ci / system (centos-10) (success)
🟢 ci / system (fedora-41) (success)
🟢 ci / system (fedora-42) (success)
🔴 ci / system (fedora-43) (failure)
🟢 ci / system (fedora-44) (success)
➖ Coverity scan / coverity (skipped)
🟢 Static code analysis / All tests are successful (success)
🟢 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.

@alexey-tikhonov
Copy link
Member

@danlavu ,

(1) please rebase / resolve conflict

(2) branches older that 2-11 do not have itng-tests, so backport won't apply cleanly for sure.
If needed, cherry-pick ifp system tests changes and open a new PR against 2-10, setting older branches as 'backport-to' (if needed.

@danlavu danlavu force-pushed the tests-rm-intg-infopipe branch from 48b7b56 to bfa48fd Compare November 18, 2025 16:43
@danlavu danlavu force-pushed the tests-rm-intg-infopipe branch from bfa48fd to 076b714 Compare November 18, 2025 17:06
@alexey-tikhonov alexey-tikhonov merged commit a276441 into SSSD:master Nov 21, 2025
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants