PR - Issue: 48851 - investigate and port TET matching rules filter tests(bug772777)

[389-ds-bot]

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

• Created at 2019-06-12 08:25:02 by aborah (@aborah-sudo)
• Merged at 2019-06-18 13:19:04

---

Investigate and port TET matching rules filter tests(bug772777)

Relates: Resolves: #1911

Author: aborah-sudo

Reviewed by: ???

[389-ds-bot]

Comment from mhonek (@kenoh) at 2019-06-12 11:53:59

There's no need for this function. Just put the people.create(...) call into the fixture below directly.

[389-ds-bot]

Comment from mhonek (@kenoh) at 2019-06-12 11:58:21

Please, don't split the very long strings like this. In tests, it is better to have an easily followable content rather than strictly following 80-chars-per-line rule. It took me quite some time even to find out how many elements this list has. Just make the filters a single string, it will get much more readable.

[389-ds-bot]

Comment from mhonek (@kenoh) at 2019-06-12 12:00:04

I have no idea what this line wants to tell...

[389-ds-bot]

Comment from aborah (@aborah-sudo) at 2019-06-12 12:42:10

1 new commit added

• Fixing Matus Honek's comments

[389-ds-bot]

Comment from aborah (@aborah-sudo) at 2019-06-12 12:44:05

2 new commits added

• Fixing Matus Honek's comments
• Issue: 48851 - investigate and port TET matching rules filter tests(bug772777)

[389-ds-bot]

Comment from aborah (@aborah-sudo) at 2019-06-12 12:44:56

@kenoh changes are done as per your suggestion

[389-ds-bot]

Comment from aborah (@aborah-sudo) at 2019-06-13 11:51:30

2 new commits added

• Fixing Matus Honek's comments
• Issue: 48851 - investigate and port TET matching rules filter tests(bug772777)

[389-ds-bot]

Comment from spichugi (@droideck) at 2019-06-17 13:33:01

I think you don't need it here at all. It is not used by the test case in any way. It can be hardcoded.

[389-ds-bot]

Comment from spichugi (@droideck) at 2019-06-17 13:35:28

Okay, I see what Matus was telling about. I think we can improve it even better because now it is not readable.
You can chop down the string in (manager=) parts. Or you can use join function and generate the string out of the list

[389-ds-bot]

Comment from spichugi (@droideck) at 2019-06-17 13:36:58

== 3 is true now but can be changed in the future.
It will be cleaner if you'll create a fresh suffix for the test case.

[389-ds-bot]

Comment from spichugi (@droideck) at 2019-06-17 13:37:49

This step is not mentioned in the docstring

[389-ds-bot]

Comment from aborah (@aborah-sudo) at 2019-06-17 14:32:58

1 new commit added

• Fixing Simon's comments

[389-ds-bot]

Comment from aborah (@aborah-sudo) at 2019-06-17 14:34:26

3 new commits added

• Fixing Simon's comments
• Fixing Matus Honek's comments
• Issue: 48851 - investigate and port TET matching rules filter tests(bug772777)

[389-ds-bot]

Comment from aborah (@aborah-sudo) at 2019-06-17 14:35:24

@droideck changes are done as per your suggestion

[389-ds-bot]

Comment from aborah (@aborah-sudo) at 2019-06-17 14:42:56

3 new commits added

• Fixing Simon's comments
• Fixing Matus Honek's comments
• Issue: 48851 - investigate and port TET matching rules filter tests(bug772777)

[389-ds-bot]

Comment from aborah (@aborah-sudo) at 2019-06-17 14:45:38

3 new commits added

• Fixing Simon's comments
• Fixing Matus Honek's comments
• Issue: 48851 - investigate and port TET matching rules filter tests(bug772777)

[389-ds-bot]

Comment from spichugi (@droideck) at 2019-06-17 14:55:00

It is really inconsistent...
Sometimes you have = in the end, sometimes in the begining, sometimes the whole (manager=uid=cnewport, dc=anuj, dc=com) is in the line, sometimes not.

[389-ds-bot]

Comment from spichugi (@droideck) at 2019-06-17 14:56:08

Why did you remove the previous steps with the user bind? I think it makes sense to test

[389-ds-bot]

Comment from aborah (@aborah-sudo) at 2019-06-17 14:58:47

Why did you remove the previous steps with the user bind? I think it makes sense to test

I need to set ACI there to give permission as its a new backend .

[389-ds-bot]

Comment from spichugi (@droideck) at 2019-06-17 15:01:55

I see no reason why not to do this.

[389-ds-bot]

Comment from aborah (@aborah-sudo) at 2019-06-17 15:16:38

1 new commit added

• Fixing Simon's Comments 2

[389-ds-bot]

Comment from aborah (@aborah-sudo) at 2019-06-17 15:17:58

@droideck changes are done as per your suggestion

[389-ds-bot]

Comment from spichugi (@droideck) at 2019-06-17 20:36:12

Don't forget to run pylint. You have a trailing whitespace here.

[389-ds-bot]

Comment from spichugi (@droideck) at 2019-06-17 20:36:41

Still, it should e more steps here according to your actual code...

[389-ds-bot]

Comment from spichugi (@droideck) at 2019-06-17 20:38:18

You already have the pytest mark.
I see no reason to name the test module dirsrvtests/tests/suites/filter/filter_bug772777_test.py.
I think you can make it a part of basic filter test suite or name it according to the actual content.

[389-ds-bot]

Comment from aborah (@aborah-sudo) at 2019-06-18 04:11:28

1 new commit added

• Fixning Simon's commnets 3

[389-ds-bot]

Comment from aborah (@aborah-sudo) at 2019-06-18 04:12:36

@droideck changes are done as per your suggestion

[389-ds-bot]

Comment from spichugi (@droideck) at 2019-06-18 12:24:21

LGTM

[389-ds-bot]

Comment from aborah (@aborah-sudo) at 2019-06-18 12:30:16

rebased onto 195ccf209984aa6a0aacbf1c706b01b65ed3fdd1

[389-ds-bot]

Comment from aborah (@aborah-sudo) at 2019-06-18 12:38:28

rebased onto 5fb5537d3ba446b35e0291fc43087db7e2cc0cf9

[389-ds-bot]

Comment from aborah (@aborah-sudo) at 2019-06-18 12:56:49

rebased onto d2566580a8ab847d5e5190faabe252feadb79978

[389-ds-bot]

Comment from aborah (@aborah-sudo) at 2019-06-18 13:09:45

rebased onto 22bc4bf73a6986e8747e2ae21901dddf982d84af

[389-ds-bot]

Comment from aborah (@aborah-sudo) at 2019-06-18 13:16:31

rebased onto 86077ec

[389-ds-bot]

Comment from spichugi (@droideck) at 2019-06-18 13:19:04

Pull-Request has been merged by droideck

[389-ds-bot]

Patch
50437.patch