- Notifications
You must be signed in to change notification settings - Fork673
feat(groups): add a list_ldap_group_links to go along with the pre ex…#2371
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
rayisbadat commentedNov 9, 2022
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
codecov-commenter commentedNov 9, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov Report
@@ Coverage Diff @@## main #2371 +/- ##==========================================- Coverage 95.95% 95.91% -0.04%========================================== Files 79 79 Lines 5286 5291 +5 ==========================================+ Hits 5072 5075 +3- Misses 214 216 +2
Flags with carried forward coverage won't be shown.Click here to find out more.
|
Uh oh!
There was an error while loading.Please reload this page.
a4456be to858dd8bCompare
lmilbaum left a comment
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.
LGTM
lmilbaum left a comment
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.
Could you please add a unit test?
I added a unit test and tox passed. But this my first time doing such a thing. So if its not right , I might need a little help on whats needed. |
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.
@lmilbaum Could you please add a unit test?
I added a unit test and tox passed. But this my first time doing such a thing. So if its not right , I might need a little help on whats needed.
@rayisbadat it's a great start! You've added a test fixture, which you can now use to also create a test case, for exampledef test_list_ldap_group_links(group, resp_list_ldap_group_links):.
It will look a bit like this existing test, but without theisinstance assert probably:
python-gitlab/tests/unit/objects/test_groups.py
Lines 248 to 251 in373b46c
| deftest_list_group_projects(group,resp_list_group_projects): | |
| projects=group.projects.list() | |
| assertisinstance(projects[0],GroupProject) | |
| assertprojects[0].path==projects_content[0]["path"] |
Just a small tip, we usually put all the fixtures (things decorated with@pytest.fixture) at the top of the module, and test cases (def test_*) after them.
…isting add_ldap_group_link and delete_ldap_group_link
7a81117 to6194085Compare@JohnVillalovos I think you wanted to have another look at this? |
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.
@nejch Looks good to me, though would like it to be squashed when it is merged in.
Thanks everyone. I did a squash and merge@JohnVillalovos |