feat(groups): add a list_ldap_group_links to go along with the pre ex…#2371
Conversation
rayisbadat
commented
Nov 9, 2022
rayisbadat
commented
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.
|
rayisbadat
force-pushed
the
feat/list_ldap_group_sync
branch
from
a4456be to
858dd8b
Compare
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. |
nejch
left a comment
nejch
left a comment
There was a problem hiding this comment.
@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 example def test_list_ldap_group_links(group, resp_list_ldap_group_links):.
It will look a bit like this existing test, but without the isinstance assert probably:
python-gitlab/tests/unit/objects/test_groups.py
Lines 248 to 251 in 373b46c
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
rayisbadat
force-pushed
the
feat/list_ldap_group_sync
branch
from
7a81117 to
6194085
Compare
|
@JohnVillalovos I think you wanted to have another look at this? |
JohnVillalovos
left a comment
JohnVillalovos
left a comment
There was a problem hiding this comment.
@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 |
6 participants
