Docstrings updated for these test suits -> ds_logs, gssapi_repl and betxn
PFA for patch.

The fixture enables dynamic plugins. So it should be named accordingly like 'dynamic_plugins'.

26 +@pytest.fixture(scope='module')
27 +def betxn_init(topology_st):

I think the setup should be consistent:

:setup: Standalone instance and enabled dynamic plugins # I'd take this
# Or
 :setup: Create a standalone instance and enable dynamic plugins

You still have BZ numbers in the docstring. It was discussed on IRC by you, me and Viktor and I think we agreed to remove it from here, but leave in pytest.mark (add pytest marks, please)

175  def test_plugin_set_invalid(topology_st):
176 -    """Bug 1273549 - Try to set some invalid values for the newly added attribute"""
177 +    """Bug 1273549 - Try to set some invalid values for nsslapd-logging-hr-timestamps-enabled

In the test_plugin_set_invalid you have two steps and one expected results. What I meant in the comments that we don't need the second step, because it says (check expectedresults)

You don't have "feature" field in every test case. Don't we need it anymore? I thought it is mandatory field...

You need to replace betxn_init with dynamic_plugins.

def test_betxt_7bit(topology_st, betxn_init):

The issue I've mentioned with Setup is still present in test_betxt_7bit case.

Please, add marks like @pytest.mark.bz1273549 as we discussed before...

commit 786b00e
Author: Amita Sharma amsharma@redhat.com
Date: Thu Sep 14 16:59:30 2017 +0530

There are some typos and sentences that should be rephrased:

233 +        1. Set 20KiB small maxbersize on master2
267 +        1. Search should be a successful
315 +    """Add a 100 entries, and run a range search. When we encounter an error
548 +        4. It should throw an TypeError exception
790 +        3. Wait some time logs to be updated
920 +    """Write one-line test case purpose (name) here

The rest looks good.

On line 59, what is the documented expected value, can we specify.
Line 64, invalid_value is a fixture, so that should be part of setup
Line 107, Function has fixtures which should be part of Setup doctsring token.
Line 182, what is max int here
Line 262,
2. Set nsslapd-listen-backlog-size to a valid value
263 + Try positive and negative.
Are these two separate steps, please fix them.

On line 59, what is the documented expected value, can we specify.

Ok, apparently I forgot to change the doc and comment while changing the code, when it was decided to check if the number of threads is just positive.

Line 64, invalid_value is a fixture, so that should be part of setup

invalid_value is the parameter and its values are mentioned in the first step.
@pytest.mark.parametrize("invalid_value", ('-2', '0', 'invalid')

Line 107, Function has fixtures which should be part of Setup doctsring token.

The same thing here. They are not fixtures and they are mentioned in steps.

Line 182, what is max int here

Ok, the same situation as on line 59, fixed.

Line 262,
2. Set nsslapd-listen-backlog-size to a valid value
263 + Try positive and negative.
Are these two separate steps, please fix them.

Fixed.

Also, I've implemented Viktor's comments after his confirmation. So feel free to set 'ack' if you're okay.

232 + :steps:
233 + 1. Set maxbersize attribute to a small value (20KiB) on master2
234 + 2. Add big value to master2
235 + 3. Add big value to master1
236 + :expectedresults:
237 + 1. maxbersize should be successfully set
238 + 2. Adding the big value to master2 is failed,
239 + 3. Adding the big value to master1 is succeed,
240 + the big value is successfully replicated to master2

Minor issues here. commas at the end in all expected results, fix the sentences please "is succeed".
the big value is successfully replicated to master2 -> does not have any step "check big value is replicate to m2" Thanks.

commit fbb0f80
Author: Simon Pichugin spichugi@redhat.com
Date: Thu Sep 14 17:29:23 2017 +0200

91 +        1. Add an entry in production with INSUFFICIENT_ACCESS

Looking on the code, it should fail if we add any entry to the production. So I think it is okay to have this step as:
1. Add an entry in production
:expectedresults
1. It should fail due to INSUFFICIENT_ACCESS # I think, it is okay to mention INSUFFICIENT_ACCESS only in expected results

The same issue for other test cases.

166 +        2. Test with no support of moddn aci
167 +            2.1 Add the moddn aci that will not be evaluated because of the config flag
168 +            2.2 Try to do modDN

Is it okay to numerate substeps like this? It's possible that import script will be broken.

175 +        2. It should fail

We need to mention why it will fail (what error). You have the same issue in other test cases.

172 +        6. Test moddn with support of moddn aci

It includes a few steps. And it includes at least two expected INSUFFICIENT_ACCESS. Could you please describe it here as you've described in previous steps for "no support of moddn aci"?
And there is clean up step in the end.

241 +        3. Try to move an entry under prod from stage

In the code we have "Now try to move an entry under except" and "new_superior = PROD_EXCEPT_DN". So I think it worth to mention it in the steps too. (that it is prod except)

264 +    """mode moddn_aci : Get Effective Rights Control

I think, it should be more descriptive, as in other test cases you have. The same for other test cases.

277 +        2. It should be entryLevelRights: 'v'

There is no step for that in the code (we don't assert 'v' in the code).

 304 +        3. It should be entryLevelRights: 'vn'

The same is here. 'should' mean that we assert something. We assert 'n' in the result only.

395 +        4. Check 'n' is not in the entryLevelRights

Assert in the test case states the opposite.

437 +        1. Try to perform MODRDN operation as anonymous on M1

There are two distinct steps in the code (with different expected exceptions - INSUFFICIENT_ACCESS and NO_SUCH_OBJECT). Also, 'expectedresults' don't mention any exception.

113 + 2. Delete the userPassword attribute with the exact value
What is the exact value here?

201 + 9. Bind as Password Admin (who is no longer an admin)
You may want to rewrite it, Password Admin who is no longer admin? -> admin rights are revoked

202 + 10. Make invalid password updates that should fail
Should fail can be part of expected results, so no need to add it in steps

206 + 14. Make the same password updates, but this time they should succeed
Again, should succeed should be part of expected result only
same with line 210

271 + 1. Add multiple passwordAdminDN attributes
This can be more clear as -> Add multiple attributes - one already exists so just try and add as second one

274 + 1. Multiple passwordAdminDN attributes should be successfully added
This is wrong. As the expected results is - It should not be added
Please recheck the code

309 + 3. Try to change password
There is one more step performed i.e. cleanup ('Bind as DM'), which is missing in docstrings

313 + 3. Subtree/User passwordChange - result
What is "off/on, on/on" here, It is not clear

355 + 7. Constraint Violation error should be raised
Please add cleanup steps of 'Bind as DM', which is missing

376 + """Make sure an entry added to ou=people has no password syntax restrictions
here also we have one more step "Bind as DM user" in finally, Please include that too

433 + 4. Set 'passwordMinLength: 9' to:
434 + a) cn=config
435 + b) ou=people policy container
So, you also used the substeps :)
I will share with you on mail, how it looks like in Polarion.
and there is also a finally block of code which is missing in steps

501 - topology_st.standalone.config.set('nsslapd-pwpolicy-local', 'off')
502 - topology_st.standalone.config.set('passwordMinCategories', '1'
we need to add these lines in setup, as they are part of fixture now

503 + :id: e8de7029-7fa6-4e96-9eb6-4a121f4c8fb3
Much Appreciated the way you have considered every tiny detail here, thanks.

About filter docstrings.

We have one common docstrings format everywhere, so probably we are better to stick to it. So instead of:

'''
Test something

We better to have:

"""Test something

Then about samll issues :)

333      Search and request attributes with extra characters.  The returned entry
334      should not have these extra characters:  "objectclass EXTRA"

Please, clean extra whitespaces (after the words 'characters')

I think, we need to have docstrings for these tests too - test_filter_logic_eq, test_filter_logic_sub, test_filter_logic_not_eq.

We need to rename "filter_logic.py" into "filter_logic_test.py" (so it will be properly discovered in betelgeuse and pytest). And we need to have proper test case names for Polarion - "test_filter_logic_eq" into "test_eq" - because we already have "filter_logic" text in test suite name.

33 +         1. Search for test users with filter (uid>=user5)
34 +         2. Search for test users with filter (uid<=user4)
38 +         1. There should be 5 users listed from USER5_DN to USER9_DN
39 +         2. There should be 15 users listed from USER0_DN to USER4_DN

Everywhere in the docstring you have "user5", "user4", etc. So I think it should be more consistent. Like: USER5_DN -> user5.

92 +    '''Test filter logic with & operators and shortcuts
133 +    '''Test filter logic with not equal to operator

In line 92, you've mentioned operator with a symbol. In line 133, "not equal to" are merged with other text and it's hard to read. Probably, it's better to use a symbol too.

For ACL test suite:

192 +        2. Test with no support of moddn aci
193 +            2.1 Add the moddn aci that will not be evaluated because of the config flag
194 +            2.2 Try to do modDN

Some substeps left

131 +        1. Try to modify DN with moddn for each value of
132 +        STAGING_DN -> PRODUCTION_DN
133 +        2. Try to modify DN with moddn for each value of
134 +        STAGING_DN -> PRODUCTION_DN with appropriate ACI

I have a concern if these lines will be imported properly. According to rST format it should be like this:

131 +        1. Try to modify DN with moddn for each value of
132 +           STAGING_DN -> PRODUCTION_DN
133 +        2. Try to modify DN with moddn for each value of
134 +           STAGING_DN -> PRODUCTION_DN with appropriate ACI

You have the same issue with the indentation in other tests.
http://docutils.sourceforge.net/docs/ref/rst/restructuredtext.html#quick-syntax-overview

commit 7b36a26
Author: Amita Sharma amsharma@redhat.com
Date: Fri Oct 6 19:13:42 2017 +0530

703 +    """Search and request attributes with extra characters.  The returned entry
704 +      should not have these extra characters:  "objectclass EXTRA"

Still, extra whitespaces.

test_filter_logic_eq function lacks :expectedresults: 2-4 steps.

It wouldn't fail, but just for your information. There is no difference between these two options. They both will be processed equally as one line:

432 +    """Test filter logic with "AND" operator
433 +       and shortcuts

432 +    """Test filter logic with "AND" operator
433 +    and shortcuts

Please, rename the test functions as I've asked before. So instead of this:

dirsrvtests/tests/suites/filter/filter_logic_test.py::test_filter_logic_and_eq PASSED
dirsrvtests/tests/suites/filter/filter_logic_test.py::test_filter_logic_or_eq PASSED

You'll have this:

dirsrvtests/tests/suites/filter/filter_logic_test.py::test_and_eq PASSED
dirsrvtests/tests/suites/filter/filter_logic_test.py::test_or_eq PASSED

And one small and not a big deal. There is only one blank line between functions. We need to have two.

commit 88fac27
Author: Amita Sharma amsharma@redhat.com
Date: Mon Oct 9 19:42:44 2017 +0530

492 + """Password policy test:
It is password policy test which is already given in file name and test name, we don't need it here again

505 + global password policy with syntax check on
nsslapd-pwpolicy-local', 'off and passwordMinCategories', '1' are also part of setup which are not mentioned
Do it as you did it on line 631

608 + 3. The attr should be changed for valid values
attr -> attribute

639 + 2. Request the control for the user
Which control

There are finally: steps too in test cases which is not mentioned in docstrings

682 + 2. Bind as the user
bind as normal user, as later we are binding with DM

680 + 1. Set passwordSendExpiringTime attribute to off or
Please mentione the value for small value here

722 + 3. Revert back user's passwordExpirationTime")
please remove ") from here

719 + 1. Expiring user's password by changing
Expire

Step and expected result don't match here ->
718 + :steps:
719 + 1. Expiring user's password by changing
720 + passwordExpirationTime timestamp

731 + 1. passwordExpirationTime is successfully changed

849 + Fine grained password policy for the user as below:
850 + ns-newpwpolicy.pl -D 'cn=Directory Manager' -w secret123
851 + -h localhost -p 389 -U 'uid=tuser,dc=example,dc=com'
You can just mention -> Fine grained password policy for the user using ns-newpwpolicy.pl

We need to rename allowed_mechs.py to allowed_mechs_test.py, (so it will be properly discovered in betelgeuse and pytest).
You may want to rename the test - "test_sasl_allowed_mechs" to remove "allowed_mechs" from it as it is in the suit name according to your last comment.

2044 + no value, '2000', '0', '-5'
no value can also be in quotes, same at other places

2047 + 2. Set nsslapd-logging-hr-timestamps-enabled to off on both masters
off on are together not clear, may be use quotes

2048 + 3. Gather all sync attempts within Counter dict, group by timestamp
what is counter dict here, not clear

2136 # Get the supported mechs. This should contain PLAIN, GSSAPI, EXTERNAL at least
Please correct "mechs" here and in other lines

2110 + 1. The list of mechanisms should be acquired
As we are asserting all 3 in code, we should write them in expected result.
code -
assert('GSSAPI' in orig_mechs)
assert('PLAIN' in orig_mechs)
assert('EXTERNAL' in orig_mechs)

2114 + 5. List should be correct
You may want to rewrite it as -> List should have - GSSAPI, PLAIN and EXTERNAL
Same at other places

These tests are not covered in docstring->
assert('GSSAPI' not in limit_mechs)
assert('GSSAPI' not in limit_mechs)

2044 + no value, '2000', '0', '-5'
no value can also be in quotes, same at other places

I've fixed it to None (so it more corresponds to the thing I put in the fixture.
I didn't put it in quotes because it can mislead the reader (we write the docstring for us in the first place). The value in quotes is a string value in the code.

The rest is fixed.

Ack to [PATCH] Issue 49381 - Refactor numerous suite docstrings - Part 2 for you @spichugi all seems pretty basic and easy :)

LGTM now. Ack from me

commit 1e28c75
Author: Simon Pichugin spichugi@redhat.com
Date: Wed Sep 20 12:01:23 2017 +0200

https://pagure.io/389-ds-base/pull-request/49708

809be53..791e5aa master -> origin/master

https://pagure.io/389-ds-base/pull-request/49861

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/2440

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.