Add tests for rebase to 1.19.17#20
Conversation
Reviewer's GuideRefactors and expands SUDO responder tests by introducing a new basic SUDO test suite, adding regression tests for sudo 1.9.17 regex and SHELL behavior, simplifying some existing sudo tests, and adjusting topology/importance and setup patterns to align with the newer sssd-test-framework APIs. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `pytest/tests/test_misc_issues.py:43` </location>
<code_context>
+ client.sssd.start()
+ result = client.auth.sudo.run("user-1", "Secret123", command="/usr/bin/env")
+ assert result.returncode == 0, "Sudo command failed!"
+ assert result.stdout.count("SHELL") == 1, "Variable SHELL is duplicated!"
</code_context>
<issue_to_address>
**suggestion:** Assertion could be more robust by checking for the exact environment variable format.
Instead of counting 'SHELL' occurrences, check for the exact line 'SHELL=/bin/zsh' or use a regex to verify the variable is present once and correctly formatted.
</issue_to_address>
### Comment 2
<location> `pytest/tests/test_misc_issues.py:35-37` </location>
<code_context>
+ 2. Variable SHELL is present only once
+ :customerscenario: True
+ """
+ client.host.ssh.run("dnf install zsh -y")
+ u = provider.user("user-1").add(uid=10001, gid=10001, shell="/bin/zsh")
+ provider.sudorule("test").add(user=u, host="ALL", command="/bin/env")
</code_context>
<issue_to_address>
**suggestion (testing):** Test setup could be made more robust by checking installation success.
Please add an assertion to check the return code of the installation command to confirm zsh was installed successfully before proceeding with the test.
```suggestion
install_result = client.host.ssh.run("dnf install zsh -y")
assert install_result.returncode == 0, "Failed to install zsh!"
u = provider.user("user-1").add(uid=10001, gid=10001, shell="/bin/zsh")
provider.sudorule("test").add(user=u, host="ALL", command="/bin/env")
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
da36e2d to
1d98f4a
Compare
b4e1e76 to
5c0b987
Compare
2517069 to
beeab77
Compare
c858cb2 to
dad7443
Compare
0f11476 to
ee38c59
Compare
ee38c59 to
a5a0fd5
Compare
|
@shridhargadekar I redid all of the assert messages in test_basic to be more descriptive. |
a5a0fd5 to
8f6045e
Compare
shridhargadekar
left a comment
There was a problem hiding this comment.
In test_basic.py,
variables u is used only once in each test case. Rest of the test case explicitly uses absolute value of user/group. Do you think sticking to either an explicit value or variable will help the continuity.
Same for a group variable g.
8f6045e to
40604ce
Compare
Add new tests for basic sudo functionality: - users - groups - commands including blacklistend and with parameters - run as user - run as group Move/refactor basic tests from test_sudo suite. Reformat with black.
40604ce to
c23148a
Compare
Add test for 'sudo passes SHELL twice'
Add tests for regex in command.
Add some basic sudo tests.
Depends on SSSD/sssd-test-framework#222
Summary by Sourcery
Add and reorganize sudo responder tests, including new coverage for basic sudo usage, regex-based sudo rules, and environment variable handling, while adjusting existing sudo tests and topology configuration.
New Features:
Enhancements:
Tests: