Don't do oneliners like this.

Is there some kind of condition you want to assert occurs here?

Doesn't useraccount have a "rename" that does modrdn for you?

Don't use raw ldap functions, get "fred" as a userAccount object then .delete() it please .

As per other reviews, why not use the generic create_test functions?

Doesn't useraccount have a "rename" that does modrdn for you?

As per the test case mod_rdn is used instead of rename

@firstyear all the changes has been done as per your suggestion , and also i have removed whole working_constans.py now only generic create_test functions are there .

Don't do onelines like this, when you keep recreating the object. Just do domain = Domain(...), then domain.set, domain.get_attr_vals, domain.add etc.

No raw delete

No raw bind

Are you attempting an anonymous bind here?

Just make a single instance of the object and re-use it please.

And here

Don't do raw modrdn_s

No raw modrdn_s

Just init the object once.

Just store "self.user_account" instead of remaking the object all the time

I'm sorry but again, I have provided so much of this feedback multiple times now. please seriously review your work more carefully. This feedback is for all your tickets for the TET patches you are trying to push. I have seen you repeat the same mistakes over and over again.

Are you attempting an anonymous bind here?

Yes , its anonymous bind

Okay, I think this is a weird looking way to achieve it. I wonder if we could do something nicer like making an actual Anonymous type in lib389/idm/account.py, and then having code like:

anon_conn = Anonymous(inst).bind()

The nice part of this is later, if we add code for manipulating Anonymous we can just do:

anon = Anonymous(inst)
anon.change_something()

@firstyear , all changes are done as per your suggestion

You only need the exception around the failing statement. This design will mask bind failures. You need to change this to:

conn = UserAccount(...).bind()
ua = UserAccount(conn, USER_DELADD)
with raises(ex):
ua.add(...)

This way the exception MUST come from ONLY the add function, which is correct.

Good hygiene could be to assert that there are no attrs before you add the attribute?

See above about scoping of raises.

And here.

And similar scoping here.

As another hygiene thing, you probably want to not use "raises Exception" but identify which exception is raised.

Good hygiene could be to assert that there are no attrs before you add the attribute?

I using assert user.get_attr_val('title') but not assert user.add("title", "architect") as user.add("title", "architect") does not return any thing . to assert in pytest function must return something , you can see in DSLdapObject :

def add(self, key, value):
    """Add an attribute with a value
    :param key: an attribute name
    :type key: str
    :param value: an attribute value
    :type value: str
    """
    self.set(key, value, action=ldap.MOD_ADD)

here add function does the work but it does not return anything .

@firstyear , all other changes are done as per your suggestion

ou.create(properties={'ou': i} ), don't need extra variable.

subtle change, we probably want

ua = UserAccount(topo.standalone)
try:
    ua.delete()

So that any exceptions in the userAccount setup are still raised. Remember, try-except should be as narrow as possible.

properties variable as before.

try-except as before.

Make those changes, and rebase to master then we can merge.

@firstyear changed and rebased

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This pull request has been cloned to Github as issue and is available here:
- https://github.com/389ds/389-ds-base/issues/3238

If you want to continue to work on the PR, please navigate to the github issue,
download the patch from the attachments and file a new pull request.

Thank you for understanding. We apologize for all inconvenience.