Tested + several other dependent tests executed as well - PASS.
The patch looks good, ACK.
----- Original Message -----
> From: "Filip Skola" <fsk...@redhat.com>
> To: "Milan Kubík" <mku...@redhat.com>
> Cc: freeipa-devel@redhat.com, "Aleš Mareček" <amare...@redhat.com>
> Sent: Monday, January 25, 2016 11:55:35 AM
> Subject: Re: [Freeipa-devel] [PATCH] 0001 Refactor test_user_plugin
>
>
>
> ----- Original Message -----
> > On 01/15/2016 03:41 PM, Filip Skola wrote:
> > > Hi,
> > >
> > > sending rebased patch on top of 58c42ddac0964a8cce7c1e1faa7516da53f028ad.
> > >
> > > Includes a "fix" for the rename-to-invalid-username issue for the new
> > > version.
> > >
> > > F.
> > >
> > > ----- Original Message -----
> > >> Hi,
> > >>
> > >> I don't know what is causing the \r\n issue. I use vim and than send
> > >> each
> > >> email with claws-mail. Didn't spot this issue when trying emailing the
> > >> patch
> > >> to my other address. I'm trying to send it from zimbra now, let me know
> > >> if
> > >> that helped pls.
> > >>
> > >> Fix for the stageuser plugin issues caused by this patch should have
> > >> been
> > >> included in the last update; I think the remaining issue is not caused
> > >> by
> > >> UserTracker changes. Please correct me, if I'm wrong.
> > >>
> > >>> There is some issue with "test_rename_to_too_long_login" test. It fails
> > >>> but
> > >>> actually this is false positive because it is possible to create login
> > >>> upto
> > >>> 255 characters. I don't know why test mentions 32 characters without
> > >>> any
> > >>> other modified setup.
> > >>> NACK for now.
> > >>> - alich -
> > >> This has been changed. This test still fails, though.
> > >>
> > >> Filip
> > >>
> > >>>
> > >>> ----- Original Message -----
> > >>>> From: "Aleš Mareček" <amare...@redhat.com>
> > >>>> To: "Filip Škola" <fsk...@redhat.com>
> > >>>> Cc: freeipa-devel@redhat.com, "Milan Kubík" <mku...@redhat.com>
> > >>>> Sent: Thursday, December 10, 2015 4:11:47 PM
> > >>>> Subject: Re: [Freeipa-devel] [PATCH] 0001 Refactor test_user_plugin
> > >>>>
> > >>>> Ah, sorry, haven't realized there had been devel list attached.
> > >>>> Ok, there is some problem with \r\n in the patch.
> > >>>> Filip, please take a look at it...
> > >>>> Thanks...
> > >>>> - alich -
> > >>>>
> > >>>> ----- Original Message -----
> > >>>>> From: "Filip Škola" <fsk...@redhat.com>
> > >>>>> To: "Aleš Mareček" <amare...@redhat.com>
> > >>>>> Cc: freeipa-devel@redhat.com, "Milan Kubík" <mku...@redhat.com>
> > >>>>> Sent: Thursday, December 10, 2015 11:29:52 AM
> > >>>>> Subject: Re: [Freeipa-devel] [PATCH] 0001 Refactor test_user_plugin
> > >>>>>
> > >>>>> Hi,
> > >>>>>
> > >>>>> this if fixed. Also issues with test_stageuser_plugin caused by
> > >>>>> UserTracker changes should be fixed here.
> > >>>>>
> > >>>>> Filip
> > >>>>>
> > >>>>>
> > >>>>> On Mon, 7 Dec 2015 09:29:31 -0500 (EST)
> > >>>>> Aleš Mareček <amare...@redhat.com> wrote:
> > >>>>>
> > >>>>>> NACK.
> > >>>>>>
> > >>>>>> $ ./make-lint
> > >>>>>> ************* Module ipatests.test_xmlrpc.test_user_plugin
> > >>>>>> ipatests/test_xmlrpc/test_user_plugin.py:42:
> > >>>>>> [E0611(no-name-in-module), ] No name 'ldaptracker' in module
> > >>>>>> 'ipatests.test_xmlrpc')
> > >>>>>>
> > >>>>>> $ grep ldaptracker ipatests/test_xmlrpc/test_user_plugin.py
> > >>>>>> from ipatests.test_xmlrpc.ldaptracker import Tracker
> > >>>>>> $ ls ipatests/test_xmlrpc/ldaptracker*
> > >>>>>> ls: cannot access ipatests/test_xmlrpc/ldaptracker*: No such file or
> > >>>>>> directory
> > >>>>>>
> > >>>>>>
> > >>>>>> ----- Original Message -----
> > >>>>>>> From: "Filip Škola" <fsk...@redhat.com>
> > >>>>>>> To: "Milan Kubík" <mku...@redhat.com>
> > >>>>>>> Cc: freeipa-devel@redhat.com
> > >>>>>>> Sent: Thursday, December 3, 2015 5:38:43 PM
> > >>>>>>> Subject: Re: [Freeipa-devel] [PATCH] 0001 Refactor test_user_plugin
> > >>>>>>>
> > >>>>>>> Hi,
> > >>>>>>>
> > >>>>>>> sending corrected version.
> > >>>>>>>
> > >>>>>>> F.
> > >>>>>>>
> > >>>>>>> On Thu, 12 Nov 2015 14:03:19 +0100
> > >>>>>>> Milan Kubík <mku...@redhat.com> wrote:
> > >>>>>>>
> > >>>>>>>> On 11/10/2015 12:13 PM, Filip Škola wrote:
> > >>>>>>>>> Hi,
> > >>>>>>>>>
> > >>>>>>>>> fixed.
> > >>>>>>>>>
> > >>>>>>>>> F.
> > >>>>>>>>>
> > >>>>>>>>> On Tue, 10 Nov 2015 10:52:45 +0100
> > >>>>>>>>> Milan Kubík <mku...@redhat.com> wrote:
> > >>>>>>>>>
> > >>>>>>>>>> On 11/09/2015 04:35 PM, Filip Škola wrote:
> > >>>>>>>>>>> Another patch was applied in the meantime.
> > >>>>>>>>>>>
> > >>>>>>>>>>> Attaching an updated version.
> > >>>>>>>>>>>
> > >>>>>>>>>>> F.
> > >>>>>>>>>>>
> > >>>>>>>>>>> On Mon, 9 Nov 2015 13:35:02 +0100
> > >>>>>>>>>>> Milan Kubík <mku...@redhat.com> wrote:
> > >>>>>>>>>>>
> > >>>>>>>>>>>> On 11/06/2015 11:32 AM, Filip Škola wrote:
> > >>>>>>>>>>>> Hi,
> > >>>>>>>>>>>> the patch doesn't apply.
> > >>>>>>>>>>>>
> > >>>>>>>>>> Please fix this.
> > >>>>>>>>>>
> > >>>>>>>>>> ipatests/test_xmlrpc/test_user_plugin.py:1419:
> > >>>>>>>>>> [E0602(undefined-variable),
> > >>>>>>>>>> TestDeniedBindWithExpiredPrincipal.teardown_class] Undefined
> > >>>>>>>>>> variable 'user1')
> > >>>>>>>>>>
> > >>>>>>>>>> Also, use the version numbers for your changed patches.
> > >>>>>>>>>>
> > >>>>>>>>>
> > >>>>>>>> Thanks for the patch. Several issues:
> > >>>>>>>>
> > >>>>>>>> 1. Use dict.items instead of dict.iteritems, for python3
> > >>>>>>>> compatibility
> > >>>>>>>>
> > >>>>>>>> 2. What is the purpose of TestPrepare class? The 'purge' methods
> > >>>>>>>> do not call any ipa commands.
> > >>>>>>>> Tracker.make_fixture should be used to make the Tracked resources
> > >>>>>>>> clean themselves up when they're out of scope.
> > >>>>>>>>
> > >>>>>>>> 3. Why reference the resources by hardcoded name if they have a
> > >>>>>>>> fixture representation?
> > >>>>>>>>
> > >>>>>>>> 4. Rewrite {create,delete}_test_group to a fixture. You may want
> > >>>>>>>> to use different scope (or not).
> > >>>>>>>>
> > >>>>>>>> 5. In `def atest_rename_to_invalid_login(self, user):` - use
> > >>>>>>>> pytest.skipif decorator and provide a reason if you must,
> > >>>>>>>> do not obfuscate method name in order not to run it.
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>
> > >>>>>>> --
> > >>>>>>> Manage your subscription for the Freeipa-devel mailing list:
> > >>>>>>> https://www.redhat.com/mailman/listinfo/freeipa-devel
> > >>>>>>> Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
> > >>>>>
> > >> --
> > >> Manage your subscription for the Freeipa-devel mailing list:
> > >> https://www.redhat.com/mailman/listinfo/freeipa-devel
> > >> Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
> > NACK, there are errors occuring that do not appear in the respective
> > test cases in the declarative test.
> > In the original module the ` Test a login name that is too long` and
> > `Try to rename to a username that is too long` do not use {add,set}attr.
> > Why do you use them?
> >
> > I'm also postponing the review of your other patches as they depend on
> > changes in this one.
> >
> > --
> > Milan Kubik
> >
> >
>
> To be honest, I have no idea why I did use setattr in those places. Update of
> 'rename' attribute was used instead in this version of the patch and that
> seems to work.
>
> Filip
--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
