You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by Alex Huang <Al...@citrix.com> on 2012/11/09 21:16:11 UTC
[DISCUSS] All checkins must include unit testing
I like to revive this topic. I punted this in 4.0 because there's just no infrastructure in place and there were a lot of users asking for a refresh of the code line.
I like to propose that starting now all commits must have unit tests and commits can be rejected on the bases that it doesnot have the appropriate unit testing.
A unit test is a test that have no external components. What this means is that it must be mocked. For example, the F5 API must be mocked up before F5 element can be checked in.
One particular problem is how do we enforce that? Today, all committers can checkin any time they want. I'm told with gerrit for example, all commits are actually held until it's been review. Should we setup infrastructure for this?
--Alex
Re: [DISCUSS] All checkins must include unit testing
Posted by prasanna <ts...@apache.org>.
I like the idea of unittests with every commit.
A few related topics I have that might be worth considering in this discussion:
1) What about unittests for the existing code? Or do we fill that gap slowly?
2) Minor but we should probably update the unittesting wiki on how to
mock our managers, daos etc?
And a few things I'm confused about:
1) Gerrit is a code review tool just like reviewboard? So - would it
really serve the purpose if committers can still check-in without
unittests?
2) Or do we enforce everyone to send in for gerrit review? That seems
counter-intuitive to the idea of committer-ship?
On 10 November 2012 18:59, Alex Huang <Al...@citrix.com> wrote:
> While I understand what you are saying, I still think anyway we can get gerrit up and running and just have people used to using it is a big plus. The fact is we really don't know what people are checking in right now. If we actually have testing, I wouldn't be as big on this. It is precisely because we don't have testing that makes me want to make sure we at least have a review system in place to catch problems.
>
> --Alex
>
>> -----Original Message-----
>> From: David Nalley [mailto:david@gnsa.us]
>> Sent: Saturday, November 10, 2012 3:41 AM
>> To: cloudstack-dev@incubator.apache.org
>> Subject: Re: [DISCUSS] All checkins must include unit testing
>>
>> On Sat, Nov 10, 2012 at 2:04 AM, Rohit Yadav <ro...@citrix.com> wrote:
>> > Can we ask ASF infra to setup gerrit, it that a possibility? If yes, that would
>> give us a lot more flexibility and quality control.
>> > Regards,
>> > Rohit
>>
>> So I am going to don my ASF infra hat for a minute, despite having no
>> real authority within infra, but just another volunteer doing work
>> there.
>> I think it is important for us to recognize our place at the ASF and
>> the reality of infra at the ASF. We are a single project, and one that
>> is still incubating. The ASF has more than 100 top level projects, and
>> scores of incubating projects. Moreover ASF infra has scarce few
>> dedicated human resources to apply - and the rest is all volunteer.
>> The number of services they are already maintaining is sizeable, and
>> it is unlikely that they will be willing to take on the installation,
>> management, and availability of yet another service, especially when
>> the following is true:
>>
>> * There is only a single project asking for the resource, and an
>> incubating one at that.
>> * It only works with git, which only a small fraction of projects at
>> the ASF use, and for which infra already has an impressive backlog of
>> tickets [1] the git backlog looks to represent almost 25% of the
>> unresolved infra tickets, and is more than double any other component
>> of ASF infra.
>>
>> What you 'might' be able to do is request a VM from infra and setup
>> gerrit on it, and take on the responsibility of keeping it updated and
>> maintaining availability yourself (and with others you manage to
>> attract to help you). In my mind there is also the question of
>> opportunity cost - we still lack much from a testing perspective
>> (which would be an important prereq for gerrit to be really effective,
>> actually being able to run tests against the code proposed) so while I
>> really like the idea of gerrit we still have a good way to go before
>> we'd be ready.
>>
>> [1]
>> https://issues.apache.org/jira/secure/IssueNavigator.jspa?reset=true&jqlQ
>> uery=project+%3D+INFRA+AND+resolution+%3D+Unresolved+AND+compo
>> nent+%3D+Git+ORDER+BY+priority+DESC&mode=hide
Re: [DISCUSS] All checkins must include unit testing
Posted by Chip Childers <ch...@sungard.com>.
On Mon, Nov 12, 2012 at 3:56 AM, Hugo Trippaers
<HT...@schubergphilis.com> wrote:
> While I really like the idea of including a unittest with every commit, I don't feel very comfortable with doing this right now. The current code coverage is pretty low and just adding new unittests for commits might even give a false sense of security. Next to that, enforcing this through technical means might prevent people from participating as we make the bar higher for anybody willing to join. I'd rather have somebody submit a patch without a unittest and having to do it myself, than not receiving the patch at all.
>
The community engagement concern is a great point Hugo. Lower the
barrier to entry, and more people will help. On the other hand, we do
need to ensure that those that accept the code take the time to deal
with test cases and documentation. With a low barrier to entry comes
an increase in committer responsibilities.
> I think the way forward would be to devote serious effort to get the current code base covered with unittests first. That way we will gain the much needed confidence in the current code, which means that we can more easily integrate changes coming in from other branches and from reviews and still have a level of certainty that stuff still works. Next to that it will provide an enormous amount of example code for would be committers and people sending patches. For a lot of people it is much easier to adapt an exisiting unittest then creating a new one from scratch, especially with a complex code base like ours. Custom injection code (like ComponentLocator) requires some knowledge to work into unittests and might be a tad difficult for people not intimately familiar with it.
>
That custom injection code is absolutely something that challenges
test writers today. I'm certainly looking forward to the Javelin
branch merging into master!
> We as the committer team should be able to govern ourselves and take the responsibility to start including unittests for exisiting code. If we can't agree to get this done as a community, implementing a technical solution will not help, but probably have the opposite effect of alienating the people. And that is something that we should not want.
+1 - absolutely
> Cheers,
>
> Hugo
>
>> -----Original Message-----
>> From: prasanna [mailto:srivatsav.prasanna@gmail.com]
>> Sent: Saturday, November 10, 2012 5:30 PM
>> To: cloudstack-dev@incubator.apache.org
>> Subject: Re: [DISCUSS] All checkins must include unit testing
>>
>> I like the idea of unittests with every commit.
>>
>> A few related topics I have that might be worth considering in this
>> discussion:
>>
>> 1) What about unittests for the existing code? Or do we fill that gap slowly?
>> 2) Minor but we should probably update the unittesting wiki on how to
>> mock our managers, daos etc?
>>
>> And a few things I'm confused about:
>>
>> 1) Gerrit is a code review tool just like reviewboard? So - would it really
>> serve the purpose if committers can still check-in without unittests?
>> 2) Or do we enforce everyone to send in for gerrit review? That seems
>> counter-intuitive to the idea of committer-ship?
>>
>> --
>> Prasanna.,
RE: [DISCUSS] All checkins must include unit testing
Posted by Hugo Trippaers <HT...@schubergphilis.com>.
While I really like the idea of including a unittest with every commit, I don't feel very comfortable with doing this right now. The current code coverage is pretty low and just adding new unittests for commits might even give a false sense of security. Next to that, enforcing this through technical means might prevent people from participating as we make the bar higher for anybody willing to join. I'd rather have somebody submit a patch without a unittest and having to do it myself, than not receiving the patch at all.
I think the way forward would be to devote serious effort to get the current code base covered with unittests first. That way we will gain the much needed confidence in the current code, which means that we can more easily integrate changes coming in from other branches and from reviews and still have a level of certainty that stuff still works. Next to that it will provide an enormous amount of example code for would be committers and people sending patches. For a lot of people it is much easier to adapt an exisiting unittest then creating a new one from scratch, especially with a complex code base like ours. Custom injection code (like ComponentLocator) requires some knowledge to work into unittests and might be a tad difficult for people not intimately familiar with it.
We as the committer team should be able to govern ourselves and take the responsibility to start including unittests for exisiting code. If we can't agree to get this done as a community, implementing a technical solution will not help, but probably have the opposite effect of alienating the people. And that is something that we should not want.
Cheers,
Hugo
> -----Original Message-----
> From: prasanna [mailto:srivatsav.prasanna@gmail.com]
> Sent: Saturday, November 10, 2012 5:30 PM
> To: cloudstack-dev@incubator.apache.org
> Subject: Re: [DISCUSS] All checkins must include unit testing
>
> I like the idea of unittests with every commit.
>
> A few related topics I have that might be worth considering in this
> discussion:
>
> 1) What about unittests for the existing code? Or do we fill that gap slowly?
> 2) Minor but we should probably update the unittesting wiki on how to
> mock our managers, daos etc?
>
> And a few things I'm confused about:
>
> 1) Gerrit is a code review tool just like reviewboard? So - would it really
> serve the purpose if committers can still check-in without unittests?
> 2) Or do we enforce everyone to send in for gerrit review? That seems
> counter-intuitive to the idea of committer-ship?
>
> --
> Prasanna.,
Re: [DISCUSS] All checkins must include unit testing
Posted by prasanna <sr...@gmail.com>.
I like the idea of unittests with every commit.
A few related topics I have that might be worth considering in this discussion:
1) What about unittests for the existing code? Or do we fill that gap slowly?
2) Minor but we should probably update the unittesting wiki on how to
mock our managers, daos etc?
And a few things I'm confused about:
1) Gerrit is a code review tool just like reviewboard? So - would it
really serve the purpose if committers can still check-in without
unittests?
2) Or do we enforce everyone to send in for gerrit review? That seems
counter-intuitive to the idea of committer-ship?
--
Prasanna.,
RE: [DISCUSS] All checkins must include unit testing
Posted by Alex Huang <Al...@citrix.com>.
While I understand what you are saying, I still think anyway we can get gerrit up and running and just have people used to using it is a big plus. The fact is we really don't know what people are checking in right now. If we actually have testing, I wouldn't be as big on this. It is precisely because we don't have testing that makes me want to make sure we at least have a review system in place to catch problems.
--Alex
> -----Original Message-----
> From: David Nalley [mailto:david@gnsa.us]
> Sent: Saturday, November 10, 2012 3:41 AM
> To: cloudstack-dev@incubator.apache.org
> Subject: Re: [DISCUSS] All checkins must include unit testing
>
> On Sat, Nov 10, 2012 at 2:04 AM, Rohit Yadav <ro...@citrix.com> wrote:
> > Can we ask ASF infra to setup gerrit, it that a possibility? If yes, that would
> give us a lot more flexibility and quality control.
> > Regards,
> > Rohit
>
> So I am going to don my ASF infra hat for a minute, despite having no
> real authority within infra, but just another volunteer doing work
> there.
> I think it is important for us to recognize our place at the ASF and
> the reality of infra at the ASF. We are a single project, and one that
> is still incubating. The ASF has more than 100 top level projects, and
> scores of incubating projects. Moreover ASF infra has scarce few
> dedicated human resources to apply - and the rest is all volunteer.
> The number of services they are already maintaining is sizeable, and
> it is unlikely that they will be willing to take on the installation,
> management, and availability of yet another service, especially when
> the following is true:
>
> * There is only a single project asking for the resource, and an
> incubating one at that.
> * It only works with git, which only a small fraction of projects at
> the ASF use, and for which infra already has an impressive backlog of
> tickets [1] the git backlog looks to represent almost 25% of the
> unresolved infra tickets, and is more than double any other component
> of ASF infra.
>
> What you 'might' be able to do is request a VM from infra and setup
> gerrit on it, and take on the responsibility of keeping it updated and
> maintaining availability yourself (and with others you manage to
> attract to help you). In my mind there is also the question of
> opportunity cost - we still lack much from a testing perspective
> (which would be an important prereq for gerrit to be really effective,
> actually being able to run tests against the code proposed) so while I
> really like the idea of gerrit we still have a good way to go before
> we'd be ready.
>
> [1]
> https://issues.apache.org/jira/secure/IssueNavigator.jspa?reset=true&jqlQ
> uery=project+%3D+INFRA+AND+resolution+%3D+Unresolved+AND+compo
> nent+%3D+Git+ORDER+BY+priority+DESC&mode=hide
Re: [DISCUSS] All checkins must include unit testing
Posted by David Nalley <da...@gnsa.us>.
On Sat, Nov 10, 2012 at 2:04 AM, Rohit Yadav <ro...@citrix.com> wrote:
> Can we ask ASF infra to setup gerrit, it that a possibility? If yes, that would give us a lot more flexibility and quality control.
> Regards,
> Rohit
So I am going to don my ASF infra hat for a minute, despite having no
real authority within infra, but just another volunteer doing work
there.
I think it is important for us to recognize our place at the ASF and
the reality of infra at the ASF. We are a single project, and one that
is still incubating. The ASF has more than 100 top level projects, and
scores of incubating projects. Moreover ASF infra has scarce few
dedicated human resources to apply - and the rest is all volunteer.
The number of services they are already maintaining is sizeable, and
it is unlikely that they will be willing to take on the installation,
management, and availability of yet another service, especially when
the following is true:
* There is only a single project asking for the resource, and an
incubating one at that.
* It only works with git, which only a small fraction of projects at
the ASF use, and for which infra already has an impressive backlog of
tickets [1] the git backlog looks to represent almost 25% of the
unresolved infra tickets, and is more than double any other component
of ASF infra.
What you 'might' be able to do is request a VM from infra and setup
gerrit on it, and take on the responsibility of keeping it updated and
maintaining availability yourself (and with others you manage to
attract to help you). In my mind there is also the question of
opportunity cost - we still lack much from a testing perspective
(which would be an important prereq for gerrit to be really effective,
actually being able to run tests against the code proposed) so while I
really like the idea of gerrit we still have a good way to go before
we'd be ready.
[1] https://issues.apache.org/jira/secure/IssueNavigator.jspa?reset=true&jqlQuery=project+%3D+INFRA+AND+resolution+%3D+Unresolved+AND+component+%3D+Git+ORDER+BY+priority+DESC&mode=hide
RE: [DISCUSS] All checkins must include unit testing
Posted by Rohit Yadav <ro...@citrix.com>.
Can we ask ASF infra to setup gerrit, it that a possibility? If yes, that would give us a lot more flexibility and quality control.
Regards,
Rohit
________________________________________
From: Alex Huang [Alex.Huang@citrix.com]
Sent: Saturday, November 10, 2012 1:46 AM
To: cloudstack-dev@incubator.apache.org
Subject: [DISCUSS] All checkins must include unit testing
I like to revive this topic. I punted this in 4.0 because there's just no infrastructure in place and there were a lot of users asking for a refresh of the code line.
I like to propose that starting now all commits must have unit tests and commits can be rejected on the bases that it doesnot have the appropriate unit testing.
A unit test is a test that have no external components. What this means is that it must be mocked. For example, the F5 API must be mocked up before F5 element can be checked in.
One particular problem is how do we enforce that? Today, all committers can checkin any time they want. I'm told with gerrit for example, all commits are actually held until it's been review. Should we setup infrastructure for this?
--Alex
RE: [DISCUSS] All checkins must include unit testing
Posted by Alex Huang <Al...@citrix.com>.
> -----Original Message-----
> From: David Nalley [mailto:david@gnsa.us]
> Sent: Friday, November 09, 2012 1:28 PM
> To: cloudstack-dev@incubator.apache.org
> Subject: Re: [DISCUSS] All checkins must include unit testing
>
> > What I meant is how do we enforce it if everyone agrees to it. So this is for
> two parts.
> >
> > 1. Do we all agree that external components should be mocked up and all
> commits have unit tests.
>
> I agree with this sentiment. At the same time I would tend to have a
> much higher level of expectation of a committer compared with a
>
> > 2. How do enforce it if we agree.
>
> While we don't have technical methods of enforcement at present, we do
> have social. Code reviews (even after the fact), vetos, etc. We
> already catch many things (license headers, etc) this way. If a unit
> test isn't forthcoming, there's always a revert.
No the problem isn't if it has a unit test. It's the coverage of the unit test. I would like to get to a point where code review is really just looking at unit tests and see what it actually covered. Is there any way to use gerrit for all commits?
--Alex
Re: [DISCUSS] All checkins must include unit testing
Posted by David Nalley <da...@gnsa.us>.
> What I meant is how do we enforce it if everyone agrees to it. So this is for two parts.
>
> 1. Do we all agree that external components should be mocked up and all commits have unit tests.
I agree with this sentiment. At the same time I would tend to have a
much higher level of expectation of a committer compared with a
> 2. How do enforce it if we agree.
While we don't have technical methods of enforcement at present, we do
have social. Code reviews (even after the fact), vetos, etc. We
already catch many things (license headers, etc) this way. If a unit
test isn't forthcoming, there's always a revert.
RE: [DISCUSS] All checkins must include unit testing
Posted by Alex Huang <Al...@citrix.com>.
> -----Original Message-----
> From: Alex Huang [mailto:Alex.Huang@citrix.com]
> Sent: Friday, November 09, 2012 12:16 PM
> To: cloudstack-dev@incubator.apache.org
> Subject: [DISCUSS] All checkins must include unit testing
>
> I like to revive this topic. I punted this in 4.0 because there's just no
> infrastructure in place and there were a lot of users asking for a refresh of
> the code line.
>
> I like to propose that starting now all commits must have unit tests and
> commits can be rejected on the bases that it doesnot have the appropriate
> unit testing.
>
> A unit test is a test that have no external components. What this means is
> that it must be mocked. For example, the F5 API must be mocked up before
> F5 element can be checked in.
>
> One particular problem is how do we enforce that? Today, all committers can
> checkin any time they want. I'm told with gerrit for example, all commits are
> actually held until it's been review. Should we setup infrastructure for this?
What I meant is how do we enforce it if everyone agrees to it. So this is for two parts.
1. Do we all agree that external components should be mocked up and all commits have unit tests.
2. How do enforce it if we agree.
--Alex