please use DEFAULT_SUFFIX instead of hardcoded suffix value

I do not understand that tests/description.
An aci denies anonymous read/write on 'givenname' attribute. My understanding is that the above line does a base search read of the entry and get the 'uid'.
This call does not select/test the aci.
I should rather expect a test where you create an entry with a 'givenname', search the entry and check that the returned entry does not contain a givenname.

Does that check that it exist a value for 'uid' attribute in the entry ?

What have I said about "use the right type for the right object?".

At the least, if you do something generic like this, use "DSLdapObject(topo.standalone, DN).delete()". Even though I hate it, it's at least "correct".

See comments on different ticket about "UserAccounts" plura form.

You can use python generators here. "a" * 9000.

If you don't do some special ou behaviour, why make this and do crazy stuff? Just leave it as ou=People, andchange the aci's to suit?

How is this test different to "test all together 1".

domain.remove_all('aci')

Did ... you read the comment about OrganisationalUnits (plural) as a constructor?

I just scrolled to a random part, and found this. Code should be:

conn = UserAccount(topo.standalone, HARRY).bind(PW_DM)
harry = UserAccount(conn, HARRY)
harry.add('sn', 'FRED')
dyngroup = Group(topo.standalone, DYNGROUP)
dyngroup.remove(...)
dyngroup.add(...)
harry.add('sn', 'Not Fred')

Can you please self review and follow my advice on this? It's now nearly 3 months and you still are not using objects (classes) correctly, and you still are not using the typed groups properly.

@firstyear self review done and all changes are done as per your suggestion , please check

Are you deleting your comments and reposting them to get me to review this .... ?

@tbordaz all changes are done , Please check .

@aborah. THe patch looks good to me and you have my ACK. Please wait for @firstyear ACK as well.

Just an external comment. It is quite difficult to evaluate to validity of ACI tests.
If you intend to port others tests, I would prefer, whenever it is possible, that you do a PR per testcase.

@tbordaz Thanks for ack , from my next PR i will limit test cases to 5 per PR , i have got around 2000 Test cases which yet to push to pagure . Thanks

@aborah I'm waiting to review this until you answer my question I asked a few days ago - are you deleting and re-posting comments on these threads?

@firstyear , few days back i was commenting "@firstyear all Changes are done. Please check"
But i was not getting any response from anybody . And i was waiting for this PR to be complete so that i can concentrate on other PRs .

While commenting for 5-6 days the same comment "@firstyear all Changes are done. Please check" , i see the same comment is appearing here continually which does not look good and have same meaning . So i have removed continuous 3 "@firstyear all Changes are done. Please check" comments.

The thing to remember @aborah is that this is the upstream project - there are different interests and time pressures on people, besides your own. It's common for me to have patches that go un-reviewed for weeks at a time, and it's not malicious, it's just that the others in the team have concerns you may not know about or see. For example, this last week I have been exclusively dealing with internal issues at SUSE, and have not had the capacity to review such a large change. (Remember what I said about making changes smaller ...) . I know that Mark often has admin duties or packaging business at RH, I know Simon probably does a lot of assistance to qe, and Matus is spread over a few projects. Thierry has to do a lot with IPA too, and he often is busy too. We all are.

Continually poking daily is a bit rude - I have not forgotten about this. I'd say, remind once a week might be fine, and certainly, deleting comments is not okay here.

In the meantime, everyone else in the project, myself included, when we wait on reviews, we can go work on other things. Please be patient and kind with people.

I will review this, when I am ready, and when I have the capacity and time to give it the attention it deserves.

@firstyear , agreed . Will remember in future. Thanks

@aborah, in my mind QE is a place where a POC becomes a product. So you can not imagine how much I valuate your effort to bring back to life former tests in lib389.

Now all tests are not equivalent. an ACI test is sensitive from a security POV but also difficult to write and to check the validity of it. So it is normal that it takes time to review, and the more there are tests to review the more difficult it is because as @firstyear said, we continuously switch from a priority to an other.

We can try making smaller review to see if it helps. I would hope more frequent ack and progress, than currently a single long pending ack.

I have to agree with William and Thierry here. Especially that with smaller patches you get quicker turnaround (roughly exponentially quicker!) -- for reviews and therefore also for your learning path.

Maybe it would make sense in dirsrvtests/tests/suites/acl/syntax_test.py not duplicating the ACI names three times each. Maybe try to rewrite to the following structure (using f-strings):

tests = [
    ('name-of-the-aci-case', f'(target = ldap:///{DEFAULT_SUFFIX}) (...'),
    ...
]
@pytest.mark.paramterize("realvalue", [a[1] for a in tests], ids=[a[0] for a in tests])
def test_all_together_part2(topo, realvalue):
    ...

It would make the content of the file more error-prone.

I think it would make sense to rename the test_all_together_part2 to something more meaningful, like test_aci_invalid_syntax.

Unless there will be more ACIs to test, with test_target_set_above_the_entry_test it does not make sense to parametrize the test, just put the value in the function.

The test_more_then_40_acl_will_crash_slapd failed for me once but I could not reproduce since. I guess no-op for now, just saying.

Please note I have not thoroughly investigated the content of the tests, so don't take this as an ack right away.

Aaaand a nit pick: it's 'syntAx' not 'syntEx'; in the commit message. :)

I have to agree with William and Thierry here. Especially that with smaller patches you get quicker turnaround (roughly exponentially quicker!) -- for reviews and therefore also for your learning path.
Maybe it would make sense in dirsrvtests/tests/suites/acl/syntax_test.py not duplicating the ACI names three times each. Maybe try to rewrite to the following structure (using f-strings):
tests = [
('name-of-the-aci-case', f'(target = ldap:///{DEFAULT_SUFFIX}) (...'),
...
]

@pytest.mark.paramterize("realvalue", [a[1] for a in tests], ids=[a[0] for a in tests])
def test_all_together_part2(topo, realvalue):
...

It would make the content of the file more error-prone.
I think it would make sense to rename the test_all_together_part2 to something more meaningful, like test_aci_invalid_syntax.
Unless there will be more ACIs to test, with test_target_set_above_the_entry_test it does not make sense to parametrize the test, just put the value in the function.
The test_more_then_40_acl_will_crash_slapd failed for me once but I could not reproduce since. I guess no-op for now, just saying.
Please note I have not thoroughly investigated the content of the tests, so don't take this as an ack right away.

@pytest.mark.paramterize("realvalue", [a[1] for a in tests], ids=[a[0] for a in tests])

It took me 20 mints to find out whats wrong with my new script . Typo(paramterize)

:)

All changes are done as per your suggestion including the commit part . Thanks

Please, catch exactly the exception that you are expecting (ldap.ALEARDY_EXISTS ?). Also, log something in that case.

And, please, apply the change to all similar cases in this script and future.

Thanks for the updates. Just a couple of notes:
- IMHO the blank lines in the list are not necessary; they don't improve readablity
- You removed the test_target_set_above_the_entry_test test function and merged it into the other test case. However, notice what was the previous way of instantiating the Domain, it used a different suffix; I think that one particular test case should come back. In my previous comment the only thing I suggested to do to this function was to drop the decorator and plug the ACI filter right into the function body.

Instead of this line, you can use the instance in uas.create(properties={. Like user = uas.create(properties={

And, please, apply the change to all similar cases in this script and future.

You can use Domain here.

A standalone instance will be more descriptive, I think

ou=PEOPLE is the default RDN for UserAccounts

You use default properties everywhere. Why not to use create_test_user function?

We have ACLPlugin object for this.

Please, run all your test cases with pytest-pylint as was advised before...

You can install it with:

pip3 install pytest-pylint

And then run with:

py.test --pylint -v dirsrvtests/tests/suites/acl/misc_test.py

You can ignore the errors regarding the fixtures like:

Redefining name 'topo' from outer scope (line 16) (redefined-outer-name)
Redefining name 'clean' from outer scope (line 46) (redefined-outer-name)
Redefining name 'aci_of_user' from outer scope (line 33) (redefined-outer-name)
Unused argument 'clean' (unused-argument)
Unused argument 'aci_of_user' (unused-argument)
Unused topology_st imported from lib389.topologies as topo (unused-import)

The rest... It will be nice to fix it. It will make the code more readable and PEP8 compliant.

Thanks for the updates. Just a couple of notes:
- IMHO the blank lines in the list are not necessary; they don't improve readablity
- You removed the test_target_set_above_the_entry_test test function and merged it into the other test case. However, notice what was the previous way of instantiating the Domain, it used a different suffix; I think that one particular test case should come back. In my previous comment the only thing I suggested to do to this function was to drop the decorator and plug the ACI filter right into the function body.

I have made the changes as per your suggestion , now it look like i have found 6 bugs here .

Please check : http://pastebin.test.redhat.com/741730

ok, so the next steps are:
- verify that the acis failing the test really do violate the syntax
- create a ticket listing the acis where syntax violation is not checked. (not just a reference to this test)

I think aci syntax evaluation is a bit sloppy in many cases, eg it checks if the bind rules are using know kewywards likle "userdn", but also accepting "userdns" or "userdnxx" - this should be fixed, but we need a ticket providing te details.

ok, so the next steps are:
- verify that the acis failing the test really do violate the syntax
- create a ticket listing the acis where syntax violation is not checked. (not just a reference to this test)
I think aci syntax evaluation is a bit sloppy in many cases, eg it checks if the bind rules are using know kewywards likle "userdn", but also accepting "userdns" or "userdnxx" - this should be fixed, but we need a ticket providing te details.

https://bugzilla.redhat.com/show_bug.cgi?id=1691473 is raised for this issue .

yes, but you reported 4 acis in the bz and you said that 6 tests were failing.

and I asked you not just to dump the test to a bugzilla but analyze and determine why the synatx is wrong, but you just did that

yes, but you reported 4 acis in the bz and you said that 6 tests were failing.
and I asked you not just to dump the test to a bugzilla but analyze and determine why the synatx is wrong, but you just did that

Bellow are the reasons for these failure:

for first aci: userdn="ldap:///{"123" * 300}";)
for second aci: userdns="ldap:///anyone";)
for third aci: userdn="ldap:////////anyone";)
for fourth aci : targetattr==*

Also mentioned in the Bug

@spichugi , all changes are done as per your suggestion

IIUC in 48d433e you removed four(six?) cases that were failing:
1. AFAICT they were not failing before the structural(!) change I requested? So where's the catch?
2. Do not remove the cases right out just because they fail, especially when you're just migrating them. Or, at least, make it clear you removed them at the rebase, with an explanation.

Also, in the same rebase you renamed some of the cases. Now, some are off by one, and some lost a meaningful name to a number. What's the reason?

IIUC in 48d433e you removed four(six?) cases that were failing:
1. AFAICT they were not failing before the structural(!) change I requested? So where's the catch?
2. Do not remove the cases right out just because they fail, especially when you're just migrating them. Or, at least, make it clear you removed them at the rebase, with an explanation.
Also, in the same rebase you renamed some of the cases. Now, some are off by one, and some lost a meaningful name to a number. What's the reason?

I have removed one case from test_aci_invalid_syntax as it is the same case that is present in test_target_set_above_the_entry_test

It's okay to leave failing cases, but mark them as pytest.xfail. It's good to have these here to show "yes, we really do expect this to go wrong".

@spichugi , all changes are done as per your suggestion

But it is not true...

• I've asked to add some logging for the try-except:pass (instead of empty pass)...
• You haven't fixed the issues from pytest-pylint
• You haven't replaced uas.create(properties) with create_test_user functions (don't forget that you can add attributes after the creation, for example, you can add mail attribute)

@spichugi , all changes are done as per your suggestion

But it is not true...

I've asked to add some logging for the try-except:pass (instead of empty pass)...
You haven't fixed the issues from pytest-pylint
You haven't replaced uas.create(properties) with create_test_user functions (don't forget that you can add attributes after the creation, for example, you can add mail attribute)

All the changes are done as per your suggestion , i have cleaned up as per as possible for pytest-pylint part

It's okay to leave failing cases, but mark them as pytest.xfail. It's good to have these here to show "yes, we really do expect this to go wrong".

I have marked the failed test cases as xfail

@spichugi , all changes are done as per your suggestion
But it is not true...
I've asked to add some logging for the try-except:pass (instead of empty pass)...
You haven't fixed the issues from pytest-pylint
You haven't replaced uas.create(properties) with create_test_user functions (don't forget that you can add attributes after the creation, for example, you can add mail attribute)

All the changes are done as per your suggestion , i have cleaned up as per as possible for pytest-pylint part

You missed a couple of suggestions... Examples (there are more of them in the report):

Variable name "ou" doesn't conform to snake_case naming style (invalid-name)
standard import "import os" should be placed before "import pytest" (wrong-import-order)
third party import "from lib389._constants import DEFAULT_SUFFIX, PW_DM" should be placed before "import ldap" (wrong-import-order)
Duplicate string formatting argument 'DEFAULT_SUFFIX', consider passing as named argument (duplicate-string-formatting-argument)
Comparison should be len(accounts.filter('(mail=*)')) == 2 (misplaced-comparison-constant)

It will make the code more readable and PEP8 compliant, as I mentioned before

@spichugi , all changes are done as per your suggestion
But it is not true...
I've asked to add some logging for the try-except:pass (instead of empty pass)...
You haven't fixed the issues from pytest-pylint
You haven't replaced uas.create(properties) with create_test_user functions (don't forget that you can add attributes after the creation, for example, you can add mail attribute)
All the changes are done as per your suggestion , i have cleaned up as per as possible for pytest-pylint part

You missed a couple of suggestions... Examples (there are more of them in the report):
Variable name "ou" doesn't conform to snake_case naming style (invalid-name)
standard import "import os" should be placed before "import pytest" (wrong-import-order)
third party import "from lib389._constants import DEFAULT_SUFFIX, PW_DM" should be placed before "import ldap" (wrong-import-order)
Duplicate string formatting argument 'DEFAULT_SUFFIX', consider passing as named argument (duplicate-string-formatting-argument)
Comparison should be len(accounts.filter('(mail=*)')) == 2 (misplaced-comparison-constant)

It will make the code more readable and PEP8 compliant, as I mentioned before

Now its all cleaned up as per the pylint .

Now its all cleaned up as per the pylint .

Oh, and could you please make the same operation on another file you have dirsrvtests/tests/suites/acl/syntax_test.py? And please, use the technic in the future PRs (and on all of the PRs that are already on review)

Overwise, very nice job fixing it! :)

P.S. you have a typo in FaileValues. And realvalue should be real_value because it is more readable.

Now its all cleaned up as per the pylint .

Oh, and could you please make the same operation on another file you have dirsrvtests/tests/suites/acl/syntax_test.py? And please, use the technic in the future PRs (and on all of the PRs that are already on review)
Overwise, very nice job fixing it! :)
P.S. you have a typo in FaileValues. And realvalue should be real_value because it is more readable.

dirsrvtests/tests/suites/acl/syntax_test.py is also cleaned up as pylint . and typo corrected

Ack from me, @spichugi do you want to do a final check and merge?

LGTM

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

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.