PR - Issue:50112 - Port ACI test suit from TET to python3(misc and syntex)

[389-ds-bot]

Cloned from Pagure Pull-Request: https://pagure.io/389-ds-base/pull-request/50178

• Created at 2019-01-24 06:26:45 by aborah (@aborah-sudo)
• Merged at 2019-03-26 11:03:11

---

Port ACI test suit from TET to python3(misc and syntex)

Resolves: #3171

Reviewed by: ???

[389-ds-bot]

Comment from aborah (@aborah-sudo) at 2019-01-25 06:18:57

rebased onto cc5e2412c55600c1cfc1125e105632a6a418212a

[389-ds-bot]

Comment from aborah (@aborah-sudo) at 2019-01-25 06:49:16

rebased onto fcba321ab72d96cc9cf3c90d6f2390f0f54b3cb2

[389-ds-bot]

Comment from aborah (@aborah-sudo) at 2019-02-04 12:20:30

rebased onto 201b45afbafd831ddbb4d950632453b2f6d6f7a1

[389-ds-bot]

Comment from aborah (@aborah-sudo) at 2019-02-05 10:32:54

rebased onto 2be1e20ceddc790c8f4589330b93104ab4380d1e

[389-ds-bot]

Comment from aborah (@aborah-sudo) at 2019-02-05 13:00:51

rebased onto fd4c4f4a1097431af5b2082164628d84107121e1

[389-ds-bot]

Comment from aborah (@aborah-sudo) at 2019-02-06 07:56:27

rebased onto af58836878e50c4fcf412764f606d93a48d8fe65

[389-ds-bot]

Comment from aborah (@aborah-sudo) at 2019-02-27 11:04:38

rebased onto 07bf7b9c3595d56b8a208b0d2715fef19bd5332f

[389-ds-bot]

Comment from aborah (@aborah-sudo) at 2019-02-27 11:08:20

rebased onto ccd5d9f62c889aee3d0963081abd66621936968b

[389-ds-bot]

Comment from aborah (@aborah-sudo) at 2019-02-27 12:23:20

rebased onto 817c696552e60bd1a33079be9cebadd785a9ccba

[389-ds-bot]

Comment from aborah (@aborah-sudo) at 2019-02-28 09:10:22

rebased onto eb8088184ce86332d195b7500550db251abd730a

[389-ds-bot]

Comment from aborah (@aborah-sudo) at 2019-02-28 09:17:49

rebased onto c8f34bb956b87792aab90922db0086b949444614

[389-ds-bot]

Comment from tbordaz (@tbordaz) at 2019-03-08 14:04:51

please use DEFAULT_SUFFIX instead of hardcoded suffix value

[389-ds-bot]

Comment from tbordaz (@tbordaz) at 2019-03-08 14:20:25

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.

[389-ds-bot]

Comment from tbordaz (@tbordaz) at 2019-03-08 14:22:08

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

[389-ds-bot]

Comment from aborah (@aborah-sudo) at 2019-03-08 15:02:25

rebased onto 381e384d6ff7493253017527b03f612d2b069299

[389-ds-bot]

Comment from aborah (@aborah-sudo) at 2019-03-08 15:47:43

rebased onto 5f95a6f796a721e33ad298ac04445c8d19259d00

[389-ds-bot]

Comment from aborah (@aborah-sudo) at 2019-03-08 16:08:43

rebased onto 5b5bc03faab2c39cfd2550195310c2e7f5c164eb

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2019-03-11 02:56:23

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".

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2019-03-11 02:56:57

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

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2019-03-11 02:58:01

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

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2019-03-11 02:59:08

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?

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2019-03-11 03:01:19

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

[389-ds-bot]

Comment from aborah (@aborah-sudo) at 2019-03-11 07:14:33

rebased onto 5474a734ea86b7919cb79261a8d9963b3a55d20a

[389-ds-bot]

Comment from aborah (@aborah-sudo) at 2019-03-11 07:17:46

rebased onto ca2d3bb9631af9f35b5f2aefc7f33a8a7e81a9ce

[389-ds-bot]

Comment from aborah (@aborah-sudo) at 2019-03-11 07:25:45

rebased onto 82a87a2642eeac955526609bae89131671b1a39b

[389-ds-bot]

Comment from aborah (@aborah-sudo) at 2019-03-12 05:52:11

rebased onto 1766259fd7de726d9a9608ac1167f9d5c13591e3

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2019-03-13 02:47:53

domain.remove_all('aci')

[389-ds-bot]

Comment from aborah (@aborah-sudo) at 2019-03-21 16:18:12

rebased onto 602a00ccacb30821a288ce9f1e0fa2c71eb1c3ab

[389-ds-bot]

Comment from aborah (@aborah-sudo) at 2019-03-21 16:20:55

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

[389-ds-bot]

Comment from lkrispen (@elkris) at 2019-03-21 17:36:55

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.

[389-ds-bot]

Comment from aborah (@aborah-sudo) at 2019-03-21 18:01:25

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 .

[389-ds-bot]

Comment from lkrispen (@elkris) at 2019-03-21 18:06:24

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

[389-ds-bot]

Comment from aborah (@aborah-sudo) at 2019-03-21 18:13:22

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

[389-ds-bot]

Comment from aborah (@aborah-sudo) at 2019-03-21 18:25:43

rebased onto 1ca49734312737de58bacec4791163dbbc927b6b

[389-ds-bot]

Comment from aborah (@aborah-sudo) at 2019-03-21 18:26:59

@droideck , all changes are done as per your suggestion

[389-ds-bot]

Comment from mhonek (@kenoh) at 2019-03-21 18:27:13

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?

[389-ds-bot]

Comment from aborah (@aborah-sudo) at 2019-03-21 18:31:41

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

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2019-03-22 01:14:13

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".

[389-ds-bot]

Comment from spichugi (@droideck) at 2019-03-22 01:40:53

@droideck , 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)

[389-ds-bot]

Comment from aborah (@aborah-sudo) at 2019-03-22 06:48:11

rebased onto 4482c014dc7d87f91e048e1bfb6cdb2ae313183e

[389-ds-bot]

Comment from aborah (@aborah-sudo) at 2019-03-22 06:49:49

@droideck , 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

[389-ds-bot]

Comment from aborah (@aborah-sudo) at 2019-03-22 07:18:56

rebased onto 8694bd312e77758c8d6e4bd4b80d69823a847e24

[389-ds-bot]

Comment from aborah (@aborah-sudo) at 2019-03-22 07:20:31

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

[389-ds-bot]

Comment from aborah (@aborah-sudo) at 2019-03-22 07:48:47

rebased onto efcf96ed4f901941e4e0b80ebfa929f09c2aabc3

[389-ds-bot]

Comment from spichugi (@droideck) at 2019-03-22 14:03:57

@droideck , 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

[389-ds-bot]

Comment from aborah (@aborah-sudo) at 2019-03-22 15:07:14

rebased onto 7c69a7741abb60e491549e773dec49c3e4073af5

[389-ds-bot]

Comment from aborah (@aborah-sudo) at 2019-03-22 15:11:04

@droideck , 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 .

[389-ds-bot]

Comment from spichugi (@droideck) at 2019-03-22 17:21:07

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.

[389-ds-bot]

Comment from aborah (@aborah-sudo) at 2019-03-22 18:05:23

rebased onto 9f92203ef3edc61602b11a9e9ca62294cc9c3590

[389-ds-bot]

Comment from aborah (@aborah-sudo) at 2019-03-22 18:09:38

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

[389-ds-bot]

Comment from aborah (@aborah-sudo) at 2019-03-24 17:19:27

rebased onto 0d645daeb70f95b26b120471349c3099b955340d

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2019-03-25 02:04:59

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

[389-ds-bot]

Comment from spichugi (@droideck) at 2019-03-25 10:56:42

LGTM

[389-ds-bot]

Comment from aborah (@aborah-sudo) at 2019-03-25 11:06:56

rebased onto 074b5794d46ddd7eacf026f2df4f0003c7ee8b13

[389-ds-bot]

Comment from aborah (@aborah-sudo) at 2019-03-25 18:48:49

rebased onto 24f8b6d

[389-ds-bot]

Comment from vashirov (@vashirov) at 2019-03-26 11:03:12

Pull-Request has been merged by vashirov

[389-ds-bot]

Patch
50178.patch