No Description Provided

Regarding technical part, you have '''ack''' from me.

Another thing:
Sankar, please, change the commit message to this:

{{{
Ticket 48050 - Add test suite to acctpolicy_plugin

Description: Verify if user account is inactivated when
accountInactivityLimit is exceeded.

https://fedorahosted.org/389/ticket/48050

Reviewed by: ?
}}}

Commit message subject usually contains a short info about what commit does.
In description you can explain the subject with more verbose.

Also, when you upload a patch for a review, please, change Review field in the ticket to '''review?'''.

Account policy plugin tests
0001-Single-test-case-for-accpolicy-tests.patch

Hi Simon, updated the patch with correct description and ticket.

I've modified the commit message to the right one.
The code looks good to me. Test passes with master branch. Ack.

To ssh://git.fedorahosted.org/git/389/ds.git
6b41a34..86bffc8 master -> master
commit 86bffc8
Author: Sankar Ramalingam sramling@redhat.com
Date: Thu Nov 24 19:34:25 2016 +0530

Added 3 test cases for account policy plugin tests
0001-Adding-3-test-cases.patch

Hi Simon, I added a new patch for review.

Rebased with master
0001-Rebased-with-master-and-added-3-test-cases.patch

Ok, it passes. Though we can improve a few thinks.

• Please, use "with pytest.raises" structure here:

{{{
try:
topology_st.standalone.simple_bind_s(userdn, USER_PW)
except ldap.LDAPError as e:
log.info('User {} is successfully inactivated : error {}'.format(userdn, e.message['desc']))
else:
raise Exception('User {} is not inactivated, expected error 19'.format(userdn))
}}}

In your code, we will have a very unverbose situation when some other error happens (not ldap.LDAPError)

• Align the docstrings to RST format.

Option #1:
elif(tochck == "Disabled"):
try:
with pytest.raises(ldap.CONSTRAINT_VIOLATION) as e:
topology.standalone.simple_bind_s(userdn, USER_PW)
except:
log.error('FAIL: User {} is not inactivated : '.format(userdn))
Option #2:
elif(tochck == "Disabled"):
with pytest.raises(ldap.CONSTRAINT_VIOLATION) as e:
topology.standalone.simple_bind_s(userdn, USER_PW)

I tried both the above cases in my code, but the results are not satisfactory for false positive cases. Say, you expect the user to be inactivated(expect error 19), and the test case fails(success, returns 0). In this case, the Option #1, returns error message, but no FAIL reported. The Option #2, reports FAIL, but it returns an error message as "Failed, DID NOT RAISE". It doesn't seems to be giving meaningful message. So, then I found the current code and tested false positive and false negative, it worked.

Replying to [comment:10 sramling]:

I tried both the above cases in my code, but the results are not satisfactory for false positive cases. Say, you expect the user to be inactivated(expect error 19), and the test case fails(success, returns 0). In this case, the Option #1, returns error message, but no FAIL reported. The Option #2, reports FAIL, but it returns an error message as "Failed, DID NOT RAISE". It doesn't seems to be giving meaningful message. So, then I found the current code and tested false positive and false negative, it worked.

Option one is not an option, it doesn't make sense.
The second option is okay. It expects ldap.CONSTRAINT_VIOLATION, but some another thing happens. Because of this test case has failed.

I've tried to change your version to this and it works for me:
{{{
with pytest.raises(ldap.CONSTRAINT_VIOLATION):
topology_st.standalone.simple_bind_s(userdn, USER_PW)
}}}
All test cases PASS.

Fixed docstrings and added pytest.raises call
0001-Added-pytest.raises-for-CONSTRAINT-VIOLATION.patch

Fixed docstrings and added pytest.raises call
0001-Fixed-docstrings-and-added-pytest.raises.patch

Sankar, could you please 'rebase -i' the last patch and the one with your main changes?
I've tried to apply it all together, but it has conflicts.

Added 3 test cases for account policy plugin tests
0001-Adding-3-test-cases-fixed-docstrings.patch

To ssh://git.fedorahosted.org/git/389/ds.git
a38d76d..b032f71 master -> master
commit b032f71
Author: Sankar Ramalingam sramling@redhat.com
Date: Wed Jan 4 17:59:20 2017 +0530

Added total 15 test cases for Account Policy plugin tests

Hi Sankar,

• first of all, please, use config operations instead of:

  BASE_CONF = "cn=config"
  topology_st.standalone.modify_s(BASE_CONF, [(ldap.MOD_REPLACE, 'passwordlockout', 'off')])
  

  As I've said before, you can find this info in the lib389-pytest-guidelines:

  topology_st.standalone.config.set('passwordlockout', 'off')
  

• You can use DN_CONFIG lib389 constant here for 'cn=config':

  ACCP_CONF = "cn=config,{}".format(ACCPOL_DN)
  

• Through the whole test suite, you catch exceptions but then you just log the info about them and continue to execute test case. For example in 'accpol_disable' fixture it doesn't make sense, because your test suite will fail after this:

  try:
      topology_st.standalone.plugins.disable(name=PLUGIN_ACCT_POLICY)
  except ldap.LDAPError as e:
      log.error('Failed to disable Account policy plugin')
  

• Please, return back the docstrings format that was created, we need it for Polarion.

• Instead of weird structure you have with 'accpol_disable' fixture inside of every 'accpol_enable' fixture, use a finalizer function inside of every 'accpol_enable' you have. It will be much more readable and you have all of it in one place.

• You don't need this. You can use the function I've told you before:

  def modify_attr(topology_st, base_dn, attr_name, attr_val):
      """Modify attribute value for Config entries"""
  

• Looks like all new test cases can be parametrized into one. They all have the similar structure.

• On a clean OpenStack Fedora machine your test cases have failed. A lot of INVALID_CREDENTIALS and CONSTRAINT_VIOLATION.

Reformatted the test cases. All test cases are passing with 1minutetip.

You can't use bare asserts for py 3

assert False
vs
assert(False)

Please use the latter.

Consider using topology_st.config.set('attr', 'value') instead of writing to cn=config directly.

subtre = "ou=groups"

Should be subtree.

+from lib389.paths import Paths

You do not need this. You must use "instance.ds_paths.<resource>". This is because the Paths module can read from ldap live, so by using Paths directly you bypass this.

Your tests pwacc_lock and userpw_reset do not assert a working bind condition, only a failing one. It's always a good idea to test positive and negative cases.

Sorry to be so pedantic, this is very important we get right.

You can't use bare asserts for py 3
assert False
vs
assert(False)

Please use the latter.

Actually, the documentation states that 'assert Statement' is a right format and proposes to use it. https://docs.python.org/3/reference/simple_stmts.html#assert

Maybe, you confused it with a 'print()'?

There is a few more things to improve:

• Please, use pytest.raises instead of this kind of code blocks:

  try:
      topology_st.standalone.simple_bind_s(userdn, USER_PASW)
  except ldap.INVALID_CREDENTIALS:
      log.info('User {} failed to login, expected INVALID_CREDENTIALS error'.format(userdn))
      assert True
  

• Also, you don't need 'assert False' in pytest.raises blocks.

• Test plan structure is a bit confusing. Sometimes you have "Set accountInactivityLimit to 12" in the steps only, sometimes in the steps and setup. The same is true about "Configure account policy plugin".
  Please, distinguish the 'setup' and 'steps' parts.

• In the test plans, the '\:id\:' field should be ':ID:' according to Amita's information.

• And regards parametrization I've changed my mind, I think it will be too complex to implement it in this case.

Incorporated all the review comments. Please review the patch and let me know if you have any comments.

• PyCharm has the ability to show you some typos (mistakes in words) and it will underline it for you with a green curvy line. Please, check your code for it, you have a few.

• line 392 in the patch, there is extra space in 'with pytest.raises (ldap.CONSTRAINT_VIOLATION):'

• In a few test cases you wrap some actions in a try-except block. For instance, in test_glnact_pwexp. And then you just log an error without throwing an exception. It shouldn't be like that.
  In the test case, you set passwordmaxage to 9 secs and then go to the next steps. If passwordmaxage wouldn't set to the value, your whole remaining test case doesn't make sense.

• Also, you have one ' Failed: DID NOT RAISE <class 'ldap.INVALID_CREDENTIALS'>' error at 'dirsrvtests/tests/suites/plugins/accpol_test.py:837'. Is it what we were talking on the meeting about?

PyCharm has the ability to show you some typos (mistakes in words) and it will underline it for you with a green curvy line. Please, check your code for it, you have a few.

Sure, thanks for that. I fixed those typos.

line 392 in the patch, there is extra space in 'with pytest.raises (ldap.CONSTRAINT_VIOLATION):'

I fixed it.

In a few test cases you wrap some actions in a try-except block. For instance, in test_glnact_pwexp. And then you just log an error without throwing an exception. It shouldn't be like that.
In the test case, you set passwordmaxage to 9 secs and then go to the next steps. If passwordmaxage wouldn't set to the value, your whole remaining test case doesn't make sense.

I added a raise.

Also, you have one ' Failed: DID NOT RAISE <class 'ldap.INVALID_CREDENTIALS'>' error at 'dirsrvtests/tests/suites/plugins/accpol_test.py:837'. Is it what we were talking on the meeting about?

Yes, this is the failure. I opened a bug for this - https://bugzilla.redhat.com/show_bug.cgi?id=1434548

I'll review this tomorrow morning. :)

All review comments incorporated.

5 +    ds_paths = Paths(serverid=topology_st.standalone.serverid,
 196 +                     instance=topology_st.standalone)

You do not need this. topology_st.standalone.ds_paths is already configured for you. Do not import Paths.

You have rolled a lot of functions like "add groups" "add user" etc. Look at lib389.idm.{groups,user}. These are much better, and portable.

I hope that helps,

5 + ds_paths = Paths(serverid=topology_st.standalone.serverid,
196 + instance=topology_st.standalone)

Done, thanks for your input.

You do not need this. topology_st.standalone.ds_paths is already configured for you. Do not import Paths.
Done.
You have rolled a lot of functions like "add groups" "add user" etc. Look at lib389.idm.{groups,user}. These are much better, and portable.
Thanks for your suggestion. I will need to modify many things if I have to use lib389.add_user/group functions. I will definitely reformat my code once I am bit more comfortable with lib389 and pytest. Since, this is my first test suite to pytest, I will go with my functions now. For sure, I will modify them at a later stage.

I hope that helps,

I also changed "subtre" to subtree all the places to make sure no grammatical mistakes :-)

Cleaned up import Paths and replaced all subtre with subtree.

Okay, you are still creating users and groups by hand though. I have asked you to review this and you haven't. Here is a more detailed explination of what I would like to see you implement.

Seriously please look at lib389/tests/idm/users_and_groups.py

You can use:

from lib389.idm.group import Groups

groups = Groups(topology.standalone, DEFAULT_SUFFIX)
    # create group
    group_properties = {
        'cn' : 'group1',
        'description' : 'testgroup'
    }

    group = groups.create(properties=group_properties)

    # user shouldn't be a member
    assert(not group.is_member(testuser.dn))

This is just an example. but it gives you a much nicer interface than raw ldap. I really urge you to use this, as this is how we want to manage objects in the future.

You can even use a bind test on users I do in the sasl test:

dirsrvtests/tests/suite/sasl/plain.py

    # Create a user
    sas = ServiceAccounts(standalone, DEFAULT_SUFFIX)
    sas._basedn = DEFAULT_SUFFIX
    sa = sas.create(properties={'cn': 'testaccount', 'userPassword': 'password'})
    # Check we can bind. This will raise exceptions if it fails.
    saconn = sa.bind('password')

At this point saconn is a new ldap connection which is bound as the service account.

I want us to start using this as it means if we change the python interface

• we see errors in tests quicker
• we don't need to waste so much time on handcrafted ldap
• tests are more concise
• you don't need to keep rolling your own user / group create helpers

I agree with William. Sankar, feel free to ping me if you have questions about the implementation. Also, I will add the instructions to the guide.

Besides that, I have no objections. One test fails, but it seems to be a bug of DS, not your test.

def test_glnact_pwexp(topology_st, accpol_global)
    ...
    log.info('Sleep for 6 secs and check if account is inactivated, expected error 19')
    time.sleep(6)
    account_status(topology_st, suffix, subtree, userid, nousrs, 0, "Disabled")
    add_time_attr(topology_st, suffix, subtree, userid, nousrs, 'lastLoginTime')
    account_status(topology_st, suffix, subtree, userid, nousrs, 0, "Enabled")
    time.sleep(4)
    account_status(topology_st, suffix, subtree, userid, nousrs, 0, "Expired")
    ...

    with pytest.raises(ldap.INVALID_CREDENTIALS):
        topology_st.standalone.simple_bind_s(userdn, USER_PASW)
        log.error('User {} password not expired , expected error 19'.format(userdn))
E                   Failed: DID NOT RAISE <class 'ldap.INVALID_CREDENTIALS'>

As another comment, looking at the doc strings I don't like how we detail every single step of the test in the doc string: This means any change to the test may diverge from the doc string.

We should be refering to the code as the primary source of details, the docstring should be general. IE do not hardcode "times" and such in the docstrings.

Thanks!

Tried my level best to incorporate all review comments. Thanks Simon and William for your great inputs.
In some test cases, I had to mention the time, since I specifically test different values for AccountInactivityLimit attribute.

There is still a bit that I would change for "perfections" sake, but I think this is good enough. Ack from me. I'll let @spichugi review this too. Thanks for following all our reviews.

For future, you can use the User objects to perform the modification and gets rather than your own get functions. It's much easier IMO.

Tests pass. Code looks good. Thank you, Sankar!

To ssh://pagure.io/389-ds-base.git
01561a1..4b5aeae master -> master
commit 4b5aeae
Author: Sankar Ramalingam sramling@redhat.com
Date: Thu Apr 13 00:40:27 2017 +0530

Adding test cases for bug #1379824

Hi Sankar,
there are a few things we can improve.
First, please, use the construction 'assert expected in output' instead of:

if expected in output:
    assert True
else:
    assert False

It will make a debugging easier.

Second, you can use ds_is_older function instead of topology_st.standalone.ds_paths.version < '1.3':

from lib389.utils import ds_is_older
if ds_is_older('1.3'):

Thanks!

Hi Simon, thanks for your review comments. I fixed both of them.

Hi Sankar,
thank you!

Please, for the future, if you want to add a test case for some particular issue, add it to the ticket where it was fixed: https://pagure.io/389-ds-base/issue/49014

Also, please use upstream numbers, not bugzilla numbers.

To ssh://pagure.io/389-ds-base.git
d5f9feb..613e871 master -> master
commit 613e871
Author: Sankar Ramalingam sramling@redhat.com
Date: Mon May 15 21:01:21 2017 +0530

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 issue has been cloned to Github and is available here:
- https://github.com/389ds/389-ds-base/issues/1381

If you want to receive further updates on the issue, please navigate to the github issue
and click on subscribe button.

Thank you for understanding. We apologize for all inconvenience.