[Freeipa-devel] patch acceptance criteria

[Petr Spacek]

On 4.12.2015 14:42, Rob Crittenden wrote:
> Lukas Slebodnik wrote:
>> > On (03/12/15 09:59), Rob Crittenden wrote:
>>> >> Lukas Slebodnik wrote:
>>>> >>> On (02/12/15 13:14), Rob Crittenden wrote:
>>>>> >>>> Is it still mandatory that tests pass the unit tests before acceptance?
>>>> >>> Unit test could be executed as part of "%check" phase in spec files.
>>>> >>> I recently added C-base unit tests there.
>>>> >>>
>>>> >>> I was not bale to run "make tests" there because many tests failed.
>>>> >>> If they require real IPA server for execution ( or lite server)
>>>> >>> then they are not unit test but integration tests.
>>>> >>> One solution would be to skip them or to usw cwrap[1] + lite server.
>>>> >>> So it can be run also in mock/koji which has many restrictions.
>>> >>
>>> >> It would represent quite a lot of work but it may be a good idea to
>>> >> investigate. Ipsilon uses cwrap for its tests so some of the
>>> >> configuration can be gleaned from that.
>>> >>
>>> >> I would definitely be opposed to this as part of the freeipa.spec in the
>> >    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> > What do you mean by this part?
>> > 
>> > Did you mean "running make tests" in spec file?
>> > If yes, could you elaborate why it's not good idea?
>> > many projects run tests in "%check"
>> > sssd, certmonger, glibc ...
>> > 
>> > Currently only C-based test are executed. And I added it only recently.
> Because it would be overkill during development. The expectation is that
> developers and reviewers run the tests before submission/acceptance. If
> they fail to do that then it will be obvious.
> 
>>> >> git tree. In Fedora it might help find problems when rawhide becomes
>>> >> Fedora.next so it would provide some value there.
>>> >>
>>>> >>>
>>>> >>> Also lint should be also part of "%check" phase and not part of
>>>> >>> ordinary build.
>>>> >>> BTW I could not see a lint[2] in fedora build at all. So I'm not sure
>>>> >>> if it is executed with upstream spec file.
>>> >>
>>> >> It isn't there because the expectation is that lint already passes as
>>> >> part of the release process. I don't see the value on running lint on
>>> >> release tarballs.
>>> >>
>> > That's just an expectation. It needn't be true. Your initial mail was about
>> > stricter review process. And automating things is best way how to
>> > enforce it. So reviewer would just build rpms and if "%check" phase
>> > will not pass then he will not continue with review.
>> > If it will be part of "%check" then you can use mock and easily ensure
>> > that test passes on stable fedora and fedora rawhide (and maybe centOS)
> By the time downstream gets a tarball it is too late to fix lint errors.
> If upstream is doing a release with lint errors then there is something
> deeply wrong with the release process. If someone wants to add it to the
> downstream spec files I'm not going to complain, I just find it
> extremely unlikely that it will provide any value, ever.

Sorry Rob, but I disagree. Lint already caught couple cases where Requires:
were not properly updated so IPA code was referencing non-existing code in
Dogtag/Custodia packages and so on.

So clearly there is some value in it.

-- 
Petr^2 Spacek