Bug Description: Port Password Policy test cases from TET to python3 part1
Relates/Fixes: https://pagure.io/389-ds-base/issue/50860
Author: aborah
Reviewed by: ???
Why not set the properties directly here? Why do you set them later?
The test docstring should reflect the feature of some component it tests. It is hard to understand which situation we test here...
Please, don't mention the placeholder names in the docstrings
It should raise INVALID_CREDENTIALS, it shouldn't be a success. Please, check all your docstrings for the related issues (the same applies to other lines I report).
INVALID_CREDENTIALS
1 new commit added
Fixing Simon's comments and test cases
Why not set the properties directly here? Why do you set them later? Changed The test docstring should reflect the feature of some component it tests. It is hard to understand which situation we test here... Please, don't mention the placeholder names in the docstrings Changed It should raise INVALID_CREDENTIALS, it shouldn't be a success. Please, check all your docstrings for the related issues (the same applies to other lines I report). Changed
Changed
The test docstring should reflect the feature of some component it tests. It is hard to understand which situation we test here... Please, don't mention the placeholder names in the docstrings
2 new commits added
Issue: 50860 - Port Password Policy test cases from TET to python3 part1
Could you please elaborate on why have you merged all of the test cases into one? (I didn't ask for that for sure...)
It is better to have separate test cases that test different features in isolated environments.
If your case, if one line in the test fails - the whole test will fail. So it may mask more issues and it will make the debugging harder. Because of this, it is better to have separate test cases for different small features.
The original test in TET was split into multiple functions, but they were not isolated. Each of the ~100 functions was part of the same test. So it was not a lot of sense to port them as they were. I asked Anuj to have a single test around the tested feature.
It is understandable and I am okay with it.
The main issue that I have is that it is very hard to follow what actually is tested.
The main feature is about 'passwordchange', okay. And then in the test case, there are no explanations about the cases we test. Instead, it has only things like changing the current password for joe and fed - success or fail. So I need to spend much more time to figure out what this is about... Why would Joe fail? Why would orly2 success? etc.
changing the current password for joe and fed
success
fail
So we either need descriptive comments or we can isolate the cases into smaller chunks of test cases and naming them properly so it's easier to follow the idea behind the test.
The original test in TET was split into multiple functions, but they were not isolated. Each of the ~100 functions was part of the same test. So it was not a lot of sense to port them as they were. I asked Anuj to have a single test around the tested feature. It is understandable and I am okay with it. The main issue that I have is that it is very hard to follow what actually is tested. The main feature is about 'passwordchange', okay. And then in the test case, there are no explanations about the cases we test. Instead, it has only things like changing the current password for joe and fed - success or fail. So I need to spend much more time to figure out what this is about... Why would Joe fail? Why would orly2 success? etc. So we either need descriptive comments or we can isolate the cases into smaller chunks of test cases and naming them properly so it's easier to follow the idea behind the test. I will take care of that . Will update the dock string
It is understandable and I am okay with it. The main issue that I have is that it is very hard to follow what actually is tested. The main feature is about 'passwordchange', okay. And then in the test case, there are no explanations about the cases we test. Instead, it has only things like changing the current password for joe and fed - success or fail. So I need to spend much more time to figure out what this is about... Why would Joe fail? Why would orly2 success? etc. So we either need descriptive comments or we can isolate the cases into smaller chunks of test cases and naming them properly so it's easier to follow the idea behind the test.
I will take care of that . Will update the dock string
The original test in TET was split into multiple functions, but they were not isolated. Each of the ~100 functions was part of the same test. So it was not a lot of sense to port them as they were. I asked Anuj to have a single test around the tested feature. It is understandable and I am okay with it. The main issue that I have is that it is very hard to follow what actually is tested. The main feature is about 'passwordchange', okay. And then in the test case, there are no explanations about the cases we test. Instead, it has only things like changing the current password for joe and fed - success or fail. So I need to spend much more time to figure out what this is about... Why would Joe fail? Why would orly2 success? etc. So we either need descriptive comments or we can isolate the cases into smaller chunks of test cases and naming them properly so it's easier to follow the idea behind the test. Doc string changed
Doc string changed
Both IDs are equal to each other
The test case fails for me... ldap.INSUFFICIENT_ACCESS: {'desc': 'Insufficient access', 'info': "Insufficient 'write' privilege to the 'userPassword' attribute of entry 'uid=dbyers,ou=dirsec,dc=example,dc=com'.\n"}
ldap.INSUFFICIENT_ACCESS: {'desc': 'Insufficient access', 'info': "Insufficient 'write' privilege to the 'userPassword' attribute of entry 'uid=dbyers,ou=dirsec,dc=example,dc=com'.\n"}
The test case fails for me... ldap.INSUFFICIENT_ACCESS: {'desc': 'Insufficient access', 'info': "Insufficient 'write' privilege to the 'userPassword' attribute of entry 'uid=dbyers,ou=dirsec,dc=example,dc=com'.\n"} both test case just passed for me . bellow are rps.
both test case just passed for me . bellow are rps.
[root@localhost Documents]# pytest-3 password_policy_test.py =============================================================================================== test session starts =============================================================================================== platform linux -- Python 3.8.2, pytest-4.6.9, py-1.8.0, pluggy-0.13.0 rootdir: /home/aborah/Documents collected 2 items
password_policy_test.py .. [100%]
================================================================================================ warnings summary =================================================================================================
[root@localhost Documents]# rpm -qa | grep 389 389-ds-base-snmp-1.4.3.5-1.fc32.x86_64 python3-lib389-1.4.3.5-1.fc32.noarch 389-ds-base-1.4.3.5-1.fc32.x86_64 389-ds-base-libs-1.4.3.5-1.fc32.x86_64 cockpit-389-ds-1.4.3.5-1.fc32.noarch 389-ds-base-legacy-tools-1.4.3.5-1.fc32.x86_64 [root@localhost Documents]#
"Both IDs are equal to each other" is still not fixed.
The rest looks good to me.
rebased onto 6235ee7e7010ade5545092ea6ac751181907184a
rebased onto 16a3da4bd981eee16b86b12d8179cd0aa565f6a6
rebased onto b87c13fd07e789627d525271058937f27a790071
rebased onto 675dc14304b8c3142557b5ce18383b67cae905bc
rebased onto 1befe9297319bd278d4008dc927dbacf1e6cea89
Pull-Request has been merged by vashirov
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/3915
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.
Pull-Request has been closed by spichugi
Bug Description: Port Password Policy test cases from TET to python3 part1
Relates/Fixes: https://pagure.io/389-ds-base/issue/50860
Author: aborah
Reviewed by: ???