Skip to content
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

Test: Port AD id mapping tests to the new framework #7172

Closed
wants to merge 1 commit into from
Closed

Test: Port AD id mapping tests to the new framework #7172

wants to merge 1 commit into from

Conversation

liswang89
Copy link
Contributor

@liswang89 liswang89 commented Feb 7, 2024

Port the following tests [https://github.com/SSSD/sssd/blob/master/src/tests/multihost/ad/test_idmap.py] to the new framework.

@alexey-tikhonov
Copy link
Member

Hi,

please add a meaningful commit message.

@andreboscatto
Copy link
Contributor

Hi @liswang89

Thanks for the contribution :) we really appreciate it. You can find more details on the standards at https://sssd.io/contrib/introduction.html -> there is a template we expect, which can be found at https://github.com/SSSD/sssd/blob/master/.git-commit-template

If you want to check other PRs to have some real examples, here are some of them: #6409 #7161 #7160

Since there is no "Git Hub" issue created for that, I don't know if we can link to our internal SSSD Jira. I'll defer it to @alexey-tikhonov or @pbrezina to tell. Should we create a GH Issue? Or just leave it blank? Can we use Resolves: SSSD Jira? Please advise.

@alexey-tikhonov
Copy link
Member

Since there is no "Git Hub" issue created for that, I don't know if we can link to our internal SSSD Jira.

No, we shouldn't link to URLs that aren't publicly accessible.

Should we create a GH Issue? Or just leave it blank? Can we use Resolves: SSSD Jira? Please advise.

Just don't add "Resolve" line in this case.
A ticket is required if this is significant bugfix/RFE and we want to have it highlighted in RNs

In this case I merely asked about description what was done (what "new tests": type of test? what does those tests do?) in a plain words.

@liswang89 liswang89 changed the title [Pytest] Port AD id mapping tests to the new framework Test: Port AD id mapping tests to the new framework Feb 7, 2024
@jakub-vavra-cz jakub-vavra-cz self-assigned this Feb 8, 2024
src/tests/system/tests/test_identity.py Outdated Show resolved Hide resolved
src/tests/system/tests/test_identity.py Outdated Show resolved Hide resolved
src/tests/system/tests/test_identity.py Outdated Show resolved Hide resolved
src/tests/system/tests/test_identity.py Outdated Show resolved Hide resolved
@jakub-vavra-cz
Copy link
Contributor

All of the user/group creation should be part of setup.
steps should match expected results 1:1
step1 should have expected result 1

@jakub-vavra-cz
Copy link
Contributor

Please add the converted marker like this one to the original test:
https://github.com/SSSD/sssd/blob/master/src/tests/multihost/basic/test_basic.py#L17
It helps to track progress and we can skip these to prevent double execution.

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.

Looks great inmplementation-wise. Thank You.
Please fix the code style, otherwise LGTM:
./tests/test_identity.py:525:61: W291 trailing whitespace
./tests/test_identity.py:542:1: W293 blank line contains whitespace
./tests/test_identity.py:547:119: W292 no newline at end of file

@danlavu
Copy link

danlavu commented Feb 14, 2024

Just Jakub's changes and a rebase.


assert client.tools.getent.group("nonposix_group") is None, 'non-posix group is returned by sssd, it should not be'
assert client.tools.getent.passwd("nonposix_user") is None, 'non-posix user is returned by sssd, it should not be'

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

Copy link
Contributor

Choose a reason for hiding this comment

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

There is some extra whitespace reported by the pep8 at L548.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jakub-vavra-cz Thanks for pointing that out. The PR has been updated.

@jakub-vavra-cz
Copy link
Contributor

Pushed PR: #7172

  • master
    • 9506b7b - Convert multihost/ad/test_idmap to test_identity
  • sssd-2-9
    • 2422af6 - Convert multihost/ad/test_idmap to test_identity

pbrezina added a commit to pbrezina/sssd that referenced this pull request Feb 22, 2024
pbrezina added a commit that referenced this pull request Feb 23, 2024
Introduced by #7172.

Reviewed-by: Alexey Tikhonov <[email protected]>
pbrezina added a commit that referenced this pull request Feb 23, 2024
Introduced by #7172.

Reviewed-by: Alexey Tikhonov <[email protected]>
(cherry picked from commit e9253e0)
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