-
Notifications
You must be signed in to change notification settings - Fork 3
Add tests for rebase to 1.19.17 #20
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Add tests for rebase to 1.19.17 #20
Conversation
Reviewer's GuideIntroduces a new pytest to verify that SSSD’s sudo responder does not duplicate the SHELL environment variable, leveraging the existing test framework roles and topologies and addressing related JIRA tickets. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
cfccb80 to
2517069
Compare
Add new tests for basic sudo functionality. Move/refactor basic tests from test_sudo suite.
2517069 to
beeab77
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