Tests: Basic coverage with tree root domain #448
Conversation
| testuser = 'treetestuser@{0}'.format(self.ad_treedomain) | ||
| result = self.legacy_client.run_command(['getent', 'passwd', testuser]) | ||
|
|
||
| testuser_regex = "treetestuser@%s:\*:%s:%s:"\ |
There was a problem hiding this comment.
Backslashes in multiline strings are discouraged by PEP-8, please enclose the whole string literal in parentheses and leave out backslashes.
Also please try to use new-style string formatting in new code.
There was a problem hiding this comment.
Thank you for review. I'll try to rewrite that part using new-style string formatting.
|
I have quickly skimmed through code and have one comment. Also, I have noticed the extreme code triplication of the test cases. I think that this warrants some refactoring first before adding tree-root domain tests. |
|
Can you be a little bit more specific about "triplication of the test cases ", please. |
|
If you look at the test cases (e.g. test_login_ipa-user, test_login_ad_user, test_login_subdomain_user are the 'best: examples) you can see that the function body is the same code copy-pasted with slight alterations so that it works for the new case. Your patch adds a fourth level of copy-pasta to the code, which is something that grieves me greatly. Clearly, you can group the common code into a private method that can be only called with the use-case specific parameters for each test case. Or you can expand the existing mixing hierarchy to achieve this. Then it would also be simpler to extend the test cases for tree-root domains. |
|
Thank you for explanation and tips. I noticed it as well and I agree that it (and not only that) worth refactoring. Yes, my PR is more or less copy-paste, because I was following existing pattern in the code. |
|
Well you still have some issues to fix, notably the failing Travis CI and the not-so nice multiline-string literal. |
|
Yes, sure I'll work on these issues |
Extend existing legacy client tests to cover test cases with tree root domain. https://fedorahosted.org/freeipa/ticket/6489
gkaihorodova
force-pushed
the
fix-6489
branch
from
ce92269
to
9ec4a2d
Compare
|
Bump for review |
|
The patch looks ok, let's hope that our CI will play nice with it. |
|
Thanks you for review. Let's hope for the best . |
|
master:
|
Tests: Basic coverage with tree root domain
Extend existing legacy client tests to cover test cases with tree root domain.
https://fedorahosted.org/freeipa/ticket/6489