Re: [Freeipa-devel] patch acceptance criteria
Petr Spacek wrote: > 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. > I'm referring to the downstream spec files (e.g. Fedora) which don't run the lint target during the build. rob -- 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
Re: [Freeipa-devel] patch acceptance criteria
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 -- 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
Re: [Freeipa-devel] patch acceptance criteria
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. >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) >I also want to keep the focus on the reviewer's responsibility to ensure >that patches do what they are supposed to and don't break things. > yes, but it does not mean that we cannot simplify/automate reviewer's tasks. IMHO, as much as possible should be done in spec file; "%check". LS -- 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
Re: [Freeipa-devel] patch acceptance criteria
On 12/02/2015 07:14 PM, Rob Crittenden wrote: Is it still mandatory that tests pass the unit tests before acceptance? I've seen a number of cases over the past couple of months where a change goes through then shortly afterward a patch to fix the tests. IMHO this should be caught in advance. Things slip through and goodness knows I've acked more than a few patches without running the full suite. I just have a feeling it has become more frequent lately. rob At 4.2 retrospective a review check list was discussed. I have a draft [1]. Comments welcome! I'm sorry, that it's only pdf at the moment. Maybe sanity checks should be less verbose, but I wanted to have it spelled out. My goal is to have both wiki page and a printable check list which can lie on a table. [1] https://pvoborni.fedorapeople.org/FreeIPAdeveloperschecklist.pdf -- Petr Vobornik -- 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
Re: [Freeipa-devel] patch acceptance criteria
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. I guess the question to ask is: since implementing lint in the dev process has a release ever gone out where things failed that would have been caught? I'm pretty sure the answer is no. >> I also want to keep the focus on the reviewer's responsibility to ensure >> that patches do what they are supposed to and don't break things. >> > yes, but it does not mean that we cannot simplify/automate reviewer's tasks. > IMHO, as much as possible should be done in spec file; "%check". It would be grossly inefficient to run the tests with every make rpms. Are you at all familiar with the IPA build and install process? It is usually build here and test over there over and over again. Running the tests with each iterative build would be horrible. It is sometimes bad enough waiting for the lint to finish. If someone wants to setup a Jenkins service to automate testing then fine. It just needs to be done in some way, and it clearly isn't on a regular basis and it is easily solved without inventing a whole new CI. rob -- 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
Re: [Freeipa-devel] patch acceptance criteria
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. 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. If someone wants to comply that it would take long time to build rpms then you might take a look how long glibc is built. http://koji.fedoraproject.org/koji/buildinfo?buildID=697271 (half an hour on x86_64) >I've seen a number of cases over the past couple of months where a >change goes through then shortly afterward a patch to fix the tests. >IMHO this should be caught in advance. > >Things slip through and goodness knows I've acked more than a few >patches without running the full suite. I just have a feeling it has >become more frequent lately. > >rob LS [1] https://cwrap.org/ [2] https://kojipkgs.fedoraproject.org//packages/freeipa/4.2.3/1.1.fc23/data/logs/x86_64/build.log -- 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
Re: [Freeipa-devel] patch acceptance criteria
On 2.12.2015 19:14, Rob Crittenden wrote: > Is it still mandatory that tests pass the unit tests before acceptance? > I've seen a number of cases over the past couple of months where a > change goes through then shortly afterward a patch to fix the tests. > IMHO this should be caught in advance. > > Things slip through and goodness knows I've acked more than a few > patches without running the full suite. I just have a feeling it has > become more frequent lately. When we are at it... An automated thingy which accepts URL to a Git repo, does all the test magic, and spits out test results without user interaction would be an awesome Christmas present! Bonus points if we can get Github integration so I can just push and have it tested automatically so I cannot forget to do that before sending the patch for review. -- Petr^2 Spacek -- 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
Re: [Freeipa-devel] patch acceptance criteria
On 12/03/2015 09:08 AM, Petr Spacek wrote: > On 2.12.2015 19:14, Rob Crittenden wrote: >> Is it still mandatory that tests pass the unit tests before acceptance? >> I've seen a number of cases over the past couple of months where a >> change goes through then shortly afterward a patch to fix the tests. >> IMHO this should be caught in advance. >> >> Things slip through and goodness knows I've acked more than a few >> patches without running the full suite. I just have a feeling it has >> become more frequent lately. > > When we are at it... An automated thingy which accepts URL to a Git repo, does > all the test magic, and spits out test results without user interaction would > be an awesome Christmas present! > > Bonus points if we can get Github integration so I can just push and have it > tested automatically so I cannot forget to do that before sending the patch > for review. +1. Having basic CI test suite run on top of a Pull Request would be awesome. -- 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
Re: [Freeipa-devel] patch acceptance criteria
Martin Kosek wrote: > On 12/03/2015 09:08 AM, Petr Spacek wrote: >> On 2.12.2015 19:14, Rob Crittenden wrote: >>> Is it still mandatory that tests pass the unit tests before acceptance? >>> I've seen a number of cases over the past couple of months where a >>> change goes through then shortly afterward a patch to fix the tests. >>> IMHO this should be caught in advance. >>> >>> Things slip through and goodness knows I've acked more than a few >>> patches without running the full suite. I just have a feeling it has >>> become more frequent lately. >> >> When we are at it... An automated thingy which accepts URL to a Git repo, >> does >> all the test magic, and spits out test results without user interaction would >> be an awesome Christmas present! >> >> Bonus points if we can get Github integration so I can just push and have it >> tested automatically so I cannot forget to do that before sending the patch >> for review. > > +1. Having basic CI test suite run on top of a Pull Request would be awesome. > I'd be happy with just the ipatests being run manually with each review. And it's then reviewer that I'm focusing on here. A developer _should_ also run the tests but part of the reviewer's responsibility is to ensure the patch does what it says it does without breaking things. rob -- 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
Re: [Freeipa-devel] patch acceptance criteria
On 3.12.2015 16:07, Rob Crittenden wrote: > Petr Spacek wrote: >> On 3.12.2015 15:34, Rob Crittenden wrote: >>> Martin Kosek wrote: On 12/03/2015 09:08 AM, Petr Spacek wrote: > On 2.12.2015 19:14, Rob Crittenden wrote: >> Is it still mandatory that tests pass the unit tests before acceptance? >> I've seen a number of cases over the past couple of months where a >> change goes through then shortly afterward a patch to fix the tests. >> IMHO this should be caught in advance. >> >> Things slip through and goodness knows I've acked more than a few >> patches without running the full suite. I just have a feeling it has >> become more frequent lately. > > When we are at it... An automated thingy which accepts URL to a Git repo, > does > all the test magic, and spits out test results without user interaction > would > be an awesome Christmas present! > > Bonus points if we can get Github integration so I can just push and have > it > tested automatically so I cannot forget to do that before sending the > patch > for review. +1. Having basic CI test suite run on top of a Pull Request would be awesome. >>> >>> I'd be happy with just the ipatests being run manually with each review. >>> >>> And it's then reviewer that I'm focusing on here. A developer _should_ >>> also run the tests but part of the reviewer's responsibility is to >>> ensure the patch does what it says it does without breaking things. >> >> Sure. I'm just saying that people are notoriously bad automation tool, so I >> would like to off-load this kind of checks to a tool which will not forget or >> be lazy :-) >> > > Sure, but it's the _job_ of the reviewer to do these things. My previous > statement is better read as: A developer _shall_ also run the tests. > > And even beyond that reviewers need to ensure that the patch works, does > it properly in the context of IPA, doesn't break anything, works in > SELinux enforcing mode, updates man pages, etc. > > In other words, when a patch breaks something, don't blame the > developer, blame the reviewer. I would blame both ;-) Again, I'm not against proper reviews, just saying that it simply does not scale, so it needs automation. -- Petr^2 Spacek -- 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
Re: [Freeipa-devel] patch acceptance criteria
On (03/12/15 15:53), Petr Spacek wrote: >On 3.12.2015 15:34, Rob Crittenden wrote: >> Martin Kosek wrote: >>> On 12/03/2015 09:08 AM, Petr Spacek wrote: On 2.12.2015 19:14, Rob Crittenden wrote: > Is it still mandatory that tests pass the unit tests before acceptance? > I've seen a number of cases over the past couple of months where a > change goes through then shortly afterward a patch to fix the tests. > IMHO this should be caught in advance. > > Things slip through and goodness knows I've acked more than a few > patches without running the full suite. I just have a feeling it has > become more frequent lately. When we are at it... An automated thingy which accepts URL to a Git repo, does all the test magic, and spits out test results without user interaction would be an awesome Christmas present! Bonus points if we can get Github integration so I can just push and have it tested automatically so I cannot forget to do that before sending the patch for review. >>> >>> +1. Having basic CI test suite run on top of a Pull Request would be >>> awesome. >>> >> >> I'd be happy with just the ipatests being run manually with each review. >> >> And it's then reviewer that I'm focusing on here. A developer _should_ >> also run the tests but part of the reviewer's responsibility is to >> ensure the patch does what it says it does without breaking things. > >Sure. I'm just saying that people are notoriously bad automation tool, so I >would like to off-load this kind of checks to a tool which will not forget or >be lazy :-) > Petr, patches are always welcomed. LS -- 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
Re: [Freeipa-devel] patch acceptance criteria
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 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. I also want to keep the focus on the reviewer's responsibility to ensure that patches do what they are supposed to and don't break things. rob > If someone wants to comply that it would take long time to build rpms > then you might take a look how long glibc is built. > http://koji.fedoraproject.org/koji/buildinfo?buildID=697271 > (half an hour on x86_64) > >> I've seen a number of cases over the past couple of months where a >> change goes through then shortly afterward a patch to fix the tests. >> IMHO this should be caught in advance. >> >> Things slip through and goodness knows I've acked more than a few >> patches without running the full suite. I just have a feeling it has >> become more frequent lately. >> >> rob > > LS > > [1] https://cwrap.org/ > [2] > https://kojipkgs.fedoraproject.org//packages/freeipa/4.2.3/1.1.fc23/data/logs/x86_64/build.log > -- 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
Re: [Freeipa-devel] patch acceptance criteria
Petr Spacek wrote: > On 3.12.2015 15:34, Rob Crittenden wrote: >> Martin Kosek wrote: >>> On 12/03/2015 09:08 AM, Petr Spacek wrote: On 2.12.2015 19:14, Rob Crittenden wrote: > Is it still mandatory that tests pass the unit tests before acceptance? > I've seen a number of cases over the past couple of months where a > change goes through then shortly afterward a patch to fix the tests. > IMHO this should be caught in advance. > > Things slip through and goodness knows I've acked more than a few > patches without running the full suite. I just have a feeling it has > become more frequent lately. When we are at it... An automated thingy which accepts URL to a Git repo, does all the test magic, and spits out test results without user interaction would be an awesome Christmas present! Bonus points if we can get Github integration so I can just push and have it tested automatically so I cannot forget to do that before sending the patch for review. >>> >>> +1. Having basic CI test suite run on top of a Pull Request would be >>> awesome. >>> >> >> I'd be happy with just the ipatests being run manually with each review. >> >> And it's then reviewer that I'm focusing on here. A developer _should_ >> also run the tests but part of the reviewer's responsibility is to >> ensure the patch does what it says it does without breaking things. > > Sure. I'm just saying that people are notoriously bad automation tool, so I > would like to off-load this kind of checks to a tool which will not forget or > be lazy :-) > Sure, but it's the _job_ of the reviewer to do these things. My previous statement is better read as: A developer _shall_ also run the tests. And even beyond that reviewers need to ensure that the patch works, does it properly in the context of IPA, doesn't break anything, works in SELinux enforcing mode, updates man pages, etc. In other words, when a patch breaks something, don't blame the developer, blame the reviewer. rob -- 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
Re: [Freeipa-devel] patch acceptance criteria
On Thu, Dec 03, 2015 at 09:59:46AM -0500, 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. SSSD uses cwrap tests as well, Nikolai wrote tests that spin up an openldap server and there are even tests that test against a Samba DC on review..so IPA developers can also look into the SSSD tree. -- 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
Re: [Freeipa-devel] patch acceptance criteria
Petr Spacek wrote: > On 3.12.2015 16:07, Rob Crittenden wrote: >> Petr Spacek wrote: >>> On 3.12.2015 15:34, Rob Crittenden wrote: Martin Kosek wrote: > On 12/03/2015 09:08 AM, Petr Spacek wrote: >> On 2.12.2015 19:14, Rob Crittenden wrote: >>> Is it still mandatory that tests pass the unit tests before acceptance? >>> I've seen a number of cases over the past couple of months where a >>> change goes through then shortly afterward a patch to fix the tests. >>> IMHO this should be caught in advance. >>> >>> Things slip through and goodness knows I've acked more than a few >>> patches without running the full suite. I just have a feeling it has >>> become more frequent lately. >> >> When we are at it... An automated thingy which accepts URL to a Git >> repo, does >> all the test magic, and spits out test results without user interaction >> would >> be an awesome Christmas present! >> >> Bonus points if we can get Github integration so I can just push and >> have it >> tested automatically so I cannot forget to do that before sending the >> patch >> for review. > > +1. Having basic CI test suite run on top of a Pull Request would be > awesome. > I'd be happy with just the ipatests being run manually with each review. And it's then reviewer that I'm focusing on here. A developer _should_ also run the tests but part of the reviewer's responsibility is to ensure the patch does what it says it does without breaking things. >>> >>> Sure. I'm just saying that people are notoriously bad automation tool, so I >>> would like to off-load this kind of checks to a tool which will not forget >>> or >>> be lazy :-) >>> >> >> Sure, but it's the _job_ of the reviewer to do these things. My previous >> statement is better read as: A developer _shall_ also run the tests. >> >> And even beyond that reviewers need to ensure that the patch works, does >> it properly in the context of IPA, doesn't break anything, works in >> SELinux enforcing mode, updates man pages, etc. >> >> In other words, when a patch breaks something, don't blame the >> developer, blame the reviewer. > > I would blame both ;-) > > Again, I'm not against proper reviews, just saying that it simply does not > scale, so it needs automation. > I haven't worked on IPA for a while so won't comment on the scaling other than to say it's always been a busy project and reviews are always a trade-off. I don't want to devolve this into automation as a panacea. There are steps for review that aren't being followed either closely enough or at all. That's what I'm trying to address. There should not be follow-up patches to fix tests that are failing. There should not be follow-up patches to add missing manpage information. And that's on the reviewer. That's all I'm saying. rob -- 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
