Skip to content

removing intg infopipe tests and made some minor updates to infopipe#8161

Merged
alexey-tikhonov merged 3 commits into
SSSD:masterfrom
danlavu:tests-rm-intg-infopipe
Nov 21, 2025
Merged

removing intg infopipe tests and made some minor updates to infopipe#8161
alexey-tikhonov merged 3 commits into
SSSD:masterfrom
danlavu:tests-rm-intg-infopipe

Conversation

@danlavu

@danlavu danlavu commented Oct 31, 2025

Copy link
Copy Markdown
Contributor

No description provided.

gemini-code-assist[bot]
gemini-code-assist Bot reviewed Oct 31, 2025
View reviewed changes

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

Copy link
Copy Markdown

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

danlavu commented Oct 31, 2025

Copy link
Copy Markdown
Contributor Author

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.

@pbrezina
pbrezina force-pushed the master branch 2 times, most recently from f5d64b3 to b854636 Compare November 4, 2025 14:27
jakub-vavra-cz
jakub-vavra-cz requested changes Nov 7, 2025
View reviewed changes

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

Copy link
Copy Markdown
Contributor

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.

Comment thread src/tests/system/tests/test_infopipe.py Outdated
@danlavu
danlavu force-pushed the tests-rm-intg-infopipe branch from 8695f9b to 0cb3eca Compare November 10, 2025 21:01
jakub-vavra-cz
jakub-vavra-cz approved these changes Nov 11, 2025
View reviewed changes

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

aplopez
aplopez reviewed Nov 13, 2025
View reviewed changes
Comment thread src/tests/system/tests/test_infopipe.py
aplopez
aplopez approved these changes Nov 13, 2025
View reviewed changes

@aplopez aplopez left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@sssd-bot

sssd-bot commented Nov 13, 2025

Copy link
Copy Markdown
Contributor

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.

@sssd-bot
sssd-bot force-pushed the tests-rm-intg-infopipe branch from 0cb3eca to 48b7b56 Compare November 13, 2025 16:24
@alexey-tikhonov

alexey-tikhonov commented Nov 14, 2025

Copy link
Copy Markdown
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
@sssd-bot sssd-bot mentioned this pull request Nov 21, 2025
Merged
@danlavu
danlavu deleted the tests-rm-intg-infopipe branch January 23, 2026 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aplopez aplopez aplopez approved these changes
@jakub-vavra-cz jakub-vavra-cz jakub-vavra-cz approved these changes
+1 more reviewer
@gemini-code-assist gemini-code-assist[bot] gemini-code-assist[bot] left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@aplopez aplopez

@jakub-vavra-cz jakub-vavra-cz

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@danlavu @sssd-bot @alexey-tikhonov @aplopez @jakub-vavra-cz