Re: [openstack-dev] When is it okay for submitters to say 'I don't want to add tests' ?
To be clear, that was a +1 for Mark's suggestion: > In cases like that, I'd be of a mind to go "+2 Awesome! Thanks for > catching this! It would be great to have a unit test for this, but it's > clear the current code is broken so I'm fine with merging the fix > without a test". You could say it's now the reviewers responsibility to > merge a test, but if that requirement then turns off reviewers even > reviewing such a patch, then that doesn't help either. On 12 November 2013 11:29, Michael Bright wrote: > > +1 also. > I spent less than half the time on my first fix (so far) understanding the > problem, reproducing it, coding it and learning about the code review > system. > > Much more than half the time was spent on reverse engineering existing > tests to be able to add new ones (which had to use features not used by the > existing tests) and asking for advice even on where to add the tests. > > It would have been more efficient for everyone had some test examples been > proposed to me. > > > > On 12 November 2013 03:34, Ed Leafe wrote: > >> On Nov 11, 2013, at 6:42 PM, Vishvananda Ishaya >> wrote: >> >> > It also gives the submitter a specific example of a well-written test, >> which >> > can be a faster way to learn than forcing them to get there via trial >> and error. >> >> +1. Implementing a policy that has as the end effect more knowledgeable >> contributors is a big win. >> >> >> -- Ed Leafe >> >> >> >> >> ___ >> OpenStack-dev mailing list >> OpenStack-dev@lists.openstack.org >> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev >> > > ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] When is it okay for submitters to say 'I don't want to add tests' ?
+1 also. I spent less than half the time on my first fix (so far) understanding the problem, reproducing it, coding it and learning about the code review system. Much more than half the time was spent on reverse engineering existing tests to be able to add new ones (which had to use features not used by the existing tests) and asking for advice even on where to add the tests. It would have been more efficient for everyone had some test examples been proposed to me. On 12 November 2013 03:34, Ed Leafe wrote: > On Nov 11, 2013, at 6:42 PM, Vishvananda Ishaya > wrote: > > > It also gives the submitter a specific example of a well-written test, > which > > can be a faster way to learn than forcing them to get there via trial > and error. > > +1. Implementing a policy that has as the end effect more knowledgeable > contributors is a big win. > > > -- Ed Leafe > > > > > ___ > OpenStack-dev mailing list > OpenStack-dev@lists.openstack.org > http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev > ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] When is it okay for submitters to say 'I don't want to add tests' ?
On Nov 11, 2013, at 6:42 PM, Vishvananda Ishaya wrote: > It also gives the submitter a specific example of a well-written test, which > can be a faster way to learn than forcing them to get there via trial and > error. +1. Implementing a policy that has as the end effect more knowledgeable contributors is a big win. -- Ed Leafe ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] When is it okay for submitters to say 'I don't want to add tests' ?
On Oct 31, 2013, at 6:56 AM, Clint Byrum wrote: > Excerpts from Mark McLoughlin's message of 2013-10-31 06:30:32 -0700: >> On Thu, 2013-10-31 at 15:37 +1300, Robert Collins wrote: >>> This is a bit of a social norms thread >>> >>> I've been consistently asking for tests in reviews for a while now, >>> and I get the occasional push-back. I think this falls into a few >>> broad camps: >>> >>> A - there is no test suite at all, adding one in unreasonable >>> B - this thing cannot be tested in this context (e.g. functional tests >>> are defined in a different tree) >>> C - this particular thing is very hard to test >>> D - testing this won't offer benefit >>> E - other things like this in the project don't have tests >>> F - submitter doesn't know how to write tests >>> G - submitter doesn't have time to write tests >> >> Nice breakdown. >> >>> Now, of these, I think it's fine not add tests in cases A, B, C in >>> combination with D, and D. >>> >>> I don't think E, F or G are sufficient reasons to merge something >>> without tests, when reviewers are asking for them. G in the special >>> case that the project really wants the patch landed - but then I'd >>> expect reviewers to not ask for tests or to volunteer that they might >>> be optional. >> >> I totally agree with the sentiment but, especially when it's a newcomer >> to the project, I try to put myself in the shoes of the patch submitter >> and double-check whether what we're asking is reasonable. >> > > Even with a long time contributor, empathy is an important part of > constructing reviews. We could make more robotic things that review for > test coverage, but we haven't, because this is a gray area. The role of > a reviewer isn't just get patches merged and stop defects. It is also to > grow the other developers. > >> For example, if someone shows up to Nova with their first OpenStack >> contribution, it fixes something which is unquestionably a bug - think >> typo like "raise NotFund('foo')" - and testing this code patch requires >> more than adding a simple new scenario to existing tests ... >> > > This goes back to my recent suggestion to help the person not with a -1 > or a +2, but with an additional patch that fixes it. > >> That, for me, is an example of "-1, we need a test! untested code is >> broken!" is really shooting the messenger, not valuing the newcomers >> contribution and risks turning that person off the project forever. >> Reviewers being overly aggressive about this where the project doesn't >> have full test coverage to begin with really makes us seem unwelcoming. >> >> In cases like that, I'd be of a mind to go "+2 Awesome! Thanks for >> catching this! It would be great to have a unit test for this, but it's >> clear the current code is broken so I'm fine with merging the fix >> without a test". You could say it's now the reviewers responsibility to >> merge a test, but if that requirement then turns off reviewers even >> reviewing such a patch, then that doesn't help either. >> > > I understand entirely why you choose this, and I think that is fine. I, > however, see this as a massive opportunity to teach. That code was only > broken because it was allowed it to be merged without tests. By letting > that situation continue, we only fix it today. The next major refactoring > has a high chance now of breaking that part of the code again. > > So, rather than +2, I suggest -1 with compassion. Engage with the > submitter. If you don't know them, take a look at how hard it would be > to write a test for the behavior and give pointers to the exact test > suite that would need to be changed, or suggest a new test suite and > point at a good example to copy. I prefer Mark's approach here. I have seen him comment that the code could use a test, and submitted a dependent patch with the test implemented. We have a very long average time from 1st patch to the gate in nova especially, and going back and forth on simple bug fixes doesn't just cost the initial submitter time, it increases the backlog and adds extra time for other reviewers as well. It also gives the submitter a specific example of a well-written test, which can be a faster way to learn than forcing them to get there via trial and error. Vish > >> So, with all of this, let's make sure we don't forget to first >> appreciate the effort that went into submitting the patch that lacks >> tests. >> > > I'm not going to claim that I've always practiced "-1 with compassion", > so thanks for reminding us all that we're not just reviewing code, we > are having a dialog with real live people. > > ___ > OpenStack-dev mailing list > OpenStack-dev@lists.openstack.org > http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] When is it okay for submitters to say 'I don't want to add tests' ?
Hey thanks a lot! - Original Message - From: "Clint Byrum" To: "openstack-dev" Sent: Thursday, October 31, 2013 7:49:55 PM Subject: Re: [openstack-dev] When is it okay for submitters to say 'I don't want to add tests' ? Excerpts from Khanh-Toan Tran's message of 2013-10-31 07:22:06 -0700: > Hi all, > > As a newbie of the community, I'm not familiar with unittest and how to use > it here. I've learned that Jenkins runs tests > everytime we submit some code. But how to write the test and what is a 'good > test' and a 'bad test'? I saw some commits > in gerrit but am unable to say if the written test is enough to judge the > code, since it is the author of the code who writes > the test. Is there a framework to follow or some rules/pratices to respect? > > Do you have some links to help me out? > This is a nice synopsis of the concept of test driven development: http://net.tutsplus.com/tutorials/python-tutorials/test-driven-development-in-python/ In OpenStack we always put tests in _base_module_name_/tests, So if you are working on nova, you can see the unit tests in: nova/tests You can generally always run the tests by installing the 'tox' python module/command on your system and running 'tox' in the root of the git repository. Projects use various testing helpers to make tests easier to read and write. The most common one is testtools. A typical test will look like this: import testtools from basemodule import submodule class TestSubmoduleFoo(testtools.TestCase): def test_foo_apple(self): self.assertEquals(1, submodule.foo('apple')) def test_foo_banana(self): self.assertEquals(0, submodule.foo('banana')) Often unit tests will include mocks and fakes to hide real world interfacing code from the unit tests. You would do well to read up on how those concepts work as well, google for 'python test mocking' and 'python test fakes'. Good luck, and #openstack-dev is always there to try and help. :) ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] When is it okay for submitters to say 'I don't want to add tests' ?
>-Original Message- >From: Chris Friesen [mailto:chris.frie...@windriver.com] >Sent: 31 October 2013 17:07 >To: openstack-dev@lists.openstack.org >Subject: Re: [openstack-dev] When is it okay for submitters to say 'I don't >want to add tests' ? > >On 10/31/2013 06:04 AM, Rosa, Andrea (HP Cloud Services) wrote: > >>> A - there is no test suite at all, adding one in unreasonable B - >>> this thing cannot be tested in this context (e.g. functional tests >>> are defined in a different tree) C - this particular thing is very >>> hard to test > >> D - testing this won't offer benefit > >> In my opinion C instead of being an acceptable reason for not having tests >> is >a symptom of one of the two things: >> 1) F => Submitter doesn't know how to write tests, in this case >> someone else can help with suggestions >> 2) The code we are trying to test is too complicated so it's time to >> refactor it >> >> And about D, In my opinion tests always offer benefits, like code coverage >or helping in understanding the code. > >I think there are actually cases where C is valid. It's difficult to test >certain >kinds of race conditions, for example, unless you have very low-level hooks >into the guts of the system in order to force the desired conditions to >reliably >occur at exactly the right time. Well depends which kind of tests we are talking about. I was talking about unit tests and I totally agree with Sandy when he said that "everything can be tested and should be". "Test certain kinds of race conditions" those kind of tests not always are unit tests, I'd consider them functional tests. Regards -- Andrea Rosa ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] When is it okay for submitters to say 'I don't want to add tests' ?
On 10/31/2013 03:24 PM, Jeremy Stanley wrote: On 2013-10-31 13:30:32 + (+), Mark McLoughlin wrote: [...] In cases like that, I'd be of a mind to go "+2 Awesome! Thanks for catching this! It would be great to have a unit test for this, but it's clear the current code is broken so I'm fine with merging the fix without a test". You could say it's now the reviewers responsibility to merge a test, but if that requirement then turns off reviewers even reviewing such a patch, then that doesn't help either. [...] I get the impression it was one of my +2s in a situation like this which spawned the thread (or at least was the straw which broke the camel's back), so apologies for stirring the beehive. Someone was trying to set up their own copy of several of our infrastructure projects, spotted a place in one where we had hard-coded something specific to our environment, and wanted to parameterize it upstream rather than paper over it on their end. The bug-reporter-turned-patch-contributor didn't know how to write a test which would confirm that parameter got passed through and we weren't performing tests yet for any similar parameters already in the daemon which I could point to as an example. I agree it's a judgement call between risking discouraging a new contributor vs. reducing test coverage further, but I was probably still a bit too hasty to suggest that we could add tests for those in a separate commit. In my case I didn't have the available time to instruct the contributor on how to write tests for this, but that also probably meant that I didn't really have time to be reviewing the change properly to begin with. I'm quite grateful to Robert for stepping in and helping walk them through it! We got more tests, I think they got a lot of benefit from the new experience as well, and I was appropriately humbled for my lax attitude over the situation which nearly allowed us to miss a great opportunity at educating another developer on the merits of test coverage. It is probably the best approach to work with a new patch submitter to show them how to write the tests. Another approach is to write a test yourself. CHeck for some precondition that shows the patch is appalied, and if it is not met, throw a Skip exception. That test should pass until their bug comes in. Then, once htier patch comes in, test test should be triggered. You can remove the skip code in a future commit. I would like to see more of this kind of coding: tests that show a feature is missing etc. We discussed it a bit in the Keystone/Client discussions where we wanted to run tests against a live sever for a client change, but couldn't really check in new tests in the server as it was after code freeze. I made the decision to push for Tempest tests there...and we broke some new ground in our workflow. One tool that is really valuable is the test coverage tool. It can show the lines of code that have no coverage, and will help confirm if a given patch is tested or not. It used to run on each commit, or so I was told. I was running it periodically for Keystone tests: here is an old run http://admiyo.fedorapeople.org/openstack/keystone/coverage/ ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] When is it okay for submitters to say 'I don't want to add tests' ?
On 2013-10-31 13:30:32 + (+), Mark McLoughlin wrote: [...] > In cases like that, I'd be of a mind to go "+2 Awesome! Thanks for > catching this! It would be great to have a unit test for this, but > it's clear the current code is broken so I'm fine with merging the > fix without a test". You could say it's now the reviewers > responsibility to merge a test, but if that requirement then turns > off reviewers even reviewing such a patch, then that doesn't help > either. [...] I get the impression it was one of my +2s in a situation like this which spawned the thread (or at least was the straw which broke the camel's back), so apologies for stirring the beehive. Someone was trying to set up their own copy of several of our infrastructure projects, spotted a place in one where we had hard-coded something specific to our environment, and wanted to parameterize it upstream rather than paper over it on their end. The bug-reporter-turned-patch-contributor didn't know how to write a test which would confirm that parameter got passed through and we weren't performing tests yet for any similar parameters already in the daemon which I could point to as an example. I agree it's a judgement call between risking discouraging a new contributor vs. reducing test coverage further, but I was probably still a bit too hasty to suggest that we could add tests for those in a separate commit. In my case I didn't have the available time to instruct the contributor on how to write tests for this, but that also probably meant that I didn't really have time to be reviewing the change properly to begin with. I'm quite grateful to Robert for stepping in and helping walk them through it! We got more tests, I think they got a lot of benefit from the new experience as well, and I was appropriately humbled for my lax attitude over the situation which nearly allowed us to miss a great opportunity at educating another developer on the merits of test coverage. -- Jeremy Stanley ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] When is it okay for submitters to say 'I don't want to add tests' ?
On 10/30/2013 11:37 PM, Robert Collins wrote: > This is a bit of a social norms thread > > I've been consistently asking for tests in reviews for a while now, > and I get the occasional push-back. I think this falls into a few > broad camps: > > A - there is no test suite at all, adding one in unreasonable > B - this thing cannot be tested in this context (e.g. functional tests > are defined in a different tree) > C - this particular thing is very hard to test > D - testing this won't offer benefit > E - other things like this in the project don't have tests > F - submitter doesn't know how to write tests > G - submitter doesn't have time to write tests > > Now, of these, I think it's fine not add tests in cases A, B, C in > combination with D, and D. > > I don't think E, F or G are sufficient reasons to merge something > without tests, when reviewers are asking for them. G in the special > case that the project really wants the patch landed - but then I'd > expect reviewers to not ask for tests or to volunteer that they might > be optional. > > Now, if I'm wrong, and folk have different norms about when to accept > 'reason X not to write tests' as a response from the submitter - > please let me know! I've done a lot of thinking around this topic [1][2] and really it comes down to this: everything can be tested and should be. There is an argument to A, but that goes beyond the scope of our use case I think. If I hear B, I would suspect the tests aren't unit tests, but are functional/integration tests (a common problem in OpenStack). Functional tests are brittle and usually have painful setup sequences. The other cases fall into the -1 camp for me. Tests required. That said, recently I was -1'ed for not updating a test, because I added code that didn't change the program flow, but introduced a new call. According to my rules, that didn't need a test, but I agreed with the logic that people would be upset if the call wasn't made (it was a notification). So a test was added. Totally valid argument. TL;DR: Tests are always required. We need to fix our tests to be proper unit tests and not functional/integration tests so it's easy to add new ones. -S [1] http://www.sandywalsh.com/2011/06/effective-units-tests-and-integration.html [2] http://www.sandywalsh.com/2011/08/pain-of-unit-tests-and-dynamically.html > -Rob > ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] When is it okay for submitters to say 'I don't want to add tests' ?
I like Steve's suggestion: > The approach the Heat team have sometimes taken in this situation is to > merge the patch, but raise a bug (targetted at the next milestone) > identifying the missing coverage. I'm (almost!) a first time contributor and I've put a fix on the backburner as I find the time to ramp up on the unit tests suite. The fix was "review"ed 2 weeks ago, requesting unit tests - very reasonable but I needed help to even start (I got no response from requests on IRC, ML, or e-mail to the bug reporter). If it wasn't for the good Upstream University people mentoring me - heh, they deserve a plug! - I'm sure I would have moved on. Thankfully, a cunning commit message provoked the guidance I needed so I just need a long weekend to get back to the tests - which I have now. I'm not sure in which cases you would/wouldn't want to split the bug for the implementation of tests but I'm sure it would help other first timers and encourage new contributors. Regards, Mike On 31 October 2013 19:51, Steven Hardy wrote: > On Thu, Oct 31, 2013 at 01:30:32PM +, Mark McLoughlin wrote: > > On Thu, 2013-10-31 at 15:37 +1300, Robert Collins wrote: > > > This is a bit of a social norms thread > > > > > > I've been consistently asking for tests in reviews for a while now, > > > and I get the occasional push-back. I think this falls into a few > > > broad camps: > > > > > > A - there is no test suite at all, adding one in unreasonable > > > B - this thing cannot be tested in this context (e.g. functional tests > > > are defined in a different tree) > > > C - this particular thing is very hard to test > > > D - testing this won't offer benefit > > > E - other things like this in the project don't have tests > > > F - submitter doesn't know how to write tests > > > G - submitter doesn't have time to write tests > > > > Nice breakdown. > > > > > Now, of these, I think it's fine not add tests in cases A, B, C in > > > combination with D, and D. > > > > > > I don't think E, F or G are sufficient reasons to merge something > > > without tests, when reviewers are asking for them. G in the special > > > case that the project really wants the patch landed - but then I'd > > > expect reviewers to not ask for tests or to volunteer that they might > > > be optional. > > > > I totally agree with the sentiment but, especially when it's a newcomer > > to the project, I try to put myself in the shoes of the patch submitter > > and double-check whether what we're asking is reasonable. > > > > For example, if someone shows up to Nova with their first OpenStack > > contribution, it fixes something which is unquestionably a bug - think > > typo like "raise NotFund('foo')" - and testing this code patch requires > > more than adding a simple new scenario to existing tests ... > > > > That, for me, is an example of "-1, we need a test! untested code is > > broken!" is really shooting the messenger, not valuing the newcomers > > contribution and risks turning that person off the project forever. > > Reviewers being overly aggressive about this where the project doesn't > > have full test coverage to begin with really makes us seem unwelcoming. > > The approach the Heat team have sometimes taken in this situation is to > merge the patch, but raise a bug (targetted at the next milestone) > identifying the missing coverage. > > This has a few benefits (provided the patch in question is sufficiently > simple that patches aren't deemed essential) > - The bug gets fixed quickly > - The coverage still gets added, and we keep track of the gaps identified > - Less chance of discouraging new submitters > - Provides some "low hanging fruit" bugs, which new contributors can pick > up > > I'm not saying we do this routinely, but it's a possible middle ground to > consider between "-1 fix all our tests" and "+2 shipit!" > > I think something that's important to recognise is that sometimes (often?) > writing tests is *hard*. I mean, look at the barriers to entry, you need > to have some idea about tox, nose, testr, mox, mock, testscenarios, etc etc > > The learning curve is really steep, and thats before you start considering > project specific test abstractions/fixtures/mocking-patterns which can all > be complex and non-obvious. > > So +1 on a bit of compassion when reviewing, particularly if the > contributor is a new or casual contributor to the project. > > Steve > > ___ > OpenStack-dev mailing list > OpenStack-dev@lists.openstack.org > http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev > ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] When is it okay for submitters to say 'I don't want to add tests' ?
On 13-10-30 10:37 PM, Robert Collins wrote: This is a bit of a social norms thread I've been consistently asking for tests in reviews for a while now, and I get the occasional push-back. I think this falls into a few broad camps: A - there is no test suite at all, adding one in unreasonable B - this thing cannot be tested in this context (e.g. functional tests are defined in a different tree) C - this particular thing is very hard to test D - testing this won't offer benefit E - other things like this in the project don't have tests F - submitter doesn't know how to write tests G - submitter doesn't have time to write tests Now, of these, I think it's fine not add tests in cases A, B, C in combination with D, and D. I don't think E, F or G are sufficient reasons to merge something without tests, when reviewers are asking for them. G in the special case that the project really wants the patch landed - but then I'd expect reviewers to not ask for tests or to volunteer that they might be optional. Now, if I'm wrong, and folk have different norms about when to accept 'reason X not to write tests' as a response from the submitter - please let me know! -Rob I recently hit option A for nodepool. My patch was accepted, but I didn't know where to start for adding the testsuite for the project. So, that said, if option A keeps coming up, then I think the obvious choice is development needs to refocus to take the option of the table. -- Paul Belanger | PolyBeacon, Inc. Jabber: paul.belan...@polybeacon.com | IRC: pabelanger (Freenode) Github: https://github.com/pabelanger | Twitter: https://twitter.com/pabelanger ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] When is it okay for submitters to say 'I don't want to add tests' ?
On Thu, Oct 31, 2013 at 01:30:32PM +, Mark McLoughlin wrote: > On Thu, 2013-10-31 at 15:37 +1300, Robert Collins wrote: > > This is a bit of a social norms thread > > > > I've been consistently asking for tests in reviews for a while now, > > and I get the occasional push-back. I think this falls into a few > > broad camps: > > > > A - there is no test suite at all, adding one in unreasonable > > B - this thing cannot be tested in this context (e.g. functional tests > > are defined in a different tree) > > C - this particular thing is very hard to test > > D - testing this won't offer benefit > > E - other things like this in the project don't have tests > > F - submitter doesn't know how to write tests > > G - submitter doesn't have time to write tests > > Nice breakdown. > > > Now, of these, I think it's fine not add tests in cases A, B, C in > > combination with D, and D. > > > > I don't think E, F or G are sufficient reasons to merge something > > without tests, when reviewers are asking for them. G in the special > > case that the project really wants the patch landed - but then I'd > > expect reviewers to not ask for tests or to volunteer that they might > > be optional. > > I totally agree with the sentiment but, especially when it's a newcomer > to the project, I try to put myself in the shoes of the patch submitter > and double-check whether what we're asking is reasonable. > > For example, if someone shows up to Nova with their first OpenStack > contribution, it fixes something which is unquestionably a bug - think > typo like "raise NotFund('foo')" - and testing this code patch requires > more than adding a simple new scenario to existing tests ... > > That, for me, is an example of "-1, we need a test! untested code is > broken!" is really shooting the messenger, not valuing the newcomers > contribution and risks turning that person off the project forever. > Reviewers being overly aggressive about this where the project doesn't > have full test coverage to begin with really makes us seem unwelcoming. The approach the Heat team have sometimes taken in this situation is to merge the patch, but raise a bug (targetted at the next milestone) identifying the missing coverage. This has a few benefits (provided the patch in question is sufficiently simple that patches aren't deemed essential) - The bug gets fixed quickly - The coverage still gets added, and we keep track of the gaps identified - Less chance of discouraging new submitters - Provides some "low hanging fruit" bugs, which new contributors can pick up I'm not saying we do this routinely, but it's a possible middle ground to consider between "-1 fix all our tests" and "+2 shipit!" I think something that's important to recognise is that sometimes (often?) writing tests is *hard*. I mean, look at the barriers to entry, you need to have some idea about tox, nose, testr, mox, mock, testscenarios, etc etc The learning curve is really steep, and thats before you start considering project specific test abstractions/fixtures/mocking-patterns which can all be complex and non-obvious. So +1 on a bit of compassion when reviewing, particularly if the contributor is a new or casual contributor to the project. Steve ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] When is it okay for submitters to say 'I don't want to add tests' ?
Excerpts from Khanh-Toan Tran's message of 2013-10-31 07:22:06 -0700: > Hi all, > > As a newbie of the community, I'm not familiar with unittest and how to use > it here. I've learned that Jenkins runs tests > everytime we submit some code. But how to write the test and what is a 'good > test' and a 'bad test'? I saw some commits > in gerrit but am unable to say if the written test is enough to judge the > code, since it is the author of the code who writes > the test. Is there a framework to follow or some rules/pratices to respect? > > Do you have some links to help me out? > This is a nice synopsis of the concept of test driven development: http://net.tutsplus.com/tutorials/python-tutorials/test-driven-development-in-python/ In OpenStack we always put tests in _base_module_name_/tests, So if you are working on nova, you can see the unit tests in: nova/tests You can generally always run the tests by installing the 'tox' python module/command on your system and running 'tox' in the root of the git repository. Projects use various testing helpers to make tests easier to read and write. The most common one is testtools. A typical test will look like this: import testtools from basemodule import submodule class TestSubmoduleFoo(testtools.TestCase): def test_foo_apple(self): self.assertEquals(1, submodule.foo('apple')) def test_foo_banana(self): self.assertEquals(0, submodule.foo('banana')) Often unit tests will include mocks and fakes to hide real world interfacing code from the unit tests. You would do well to read up on how those concepts work as well, google for 'python test mocking' and 'python test fakes'. Good luck, and #openstack-dev is always there to try and help. :) ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] When is it okay for submitters to say 'I don't want to add tests' ?
On 2013-10-31 09:05, Kyle Mestery (kmestery) wrote: On Oct 31, 2013, at 8:56 AM, Clint Byrum wrote: Excerpts from Mark McLoughlin's message of 2013-10-31 06:30:32 -0700: On Thu, 2013-10-31 at 15:37 +1300, Robert Collins wrote: This is a bit of a social norms thread I've been consistently asking for tests in reviews for a while now, and I get the occasional push-back. I think this falls into a few broad camps: A - there is no test suite at all, adding one in unreasonable B - this thing cannot be tested in this context (e.g. functional tests are defined in a different tree) C - this particular thing is very hard to test D - testing this won't offer benefit E - other things like this in the project don't have tests F - submitter doesn't know how to write tests G - submitter doesn't have time to write tests Nice breakdown. Now, of these, I think it's fine not add tests in cases A, B, C in combination with D, and D. I don't think E, F or G are sufficient reasons to merge something without tests, when reviewers are asking for them. G in the special case that the project really wants the patch landed - but then I'd expect reviewers to not ask for tests or to volunteer that they might be optional. I totally agree with the sentiment but, especially when it's a newcomer to the project, I try to put myself in the shoes of the patch submitter and double-check whether what we're asking is reasonable. Even with a long time contributor, empathy is an important part of constructing reviews. We could make more robotic things that review for test coverage, but we haven't, because this is a gray area. The role of a reviewer isn't just get patches merged and stop defects. It is also to grow the other developers. For example, if someone shows up to Nova with their first OpenStack contribution, it fixes something which is unquestionably a bug - think typo like "raise NotFund('foo')" - and testing this code patch requires more than adding a simple new scenario to existing tests ... This goes back to my recent suggestion to help the person not with a -1 or a +2, but with an additional patch that fixes it. That, for me, is an example of "-1, we need a test! untested code is broken!" is really shooting the messenger, not valuing the newcomers contribution and risks turning that person off the project forever. Reviewers being overly aggressive about this where the project doesn't have full test coverage to begin with really makes us seem unwelcoming. In cases like that, I'd be of a mind to go "+2 Awesome! Thanks for catching this! It would be great to have a unit test for this, but it's clear the current code is broken so I'm fine with merging the fix without a test". You could say it's now the reviewers responsibility to merge a test, but if that requirement then turns off reviewers even reviewing such a patch, then that doesn't help either. I understand entirely why you choose this, and I think that is fine. I, however, see this as a massive opportunity to teach. That code was only broken because it was allowed it to be merged without tests. By letting that situation continue, we only fix it today. The next major refactoring has a high chance now of breaking that part of the code again. So, rather than +2, I suggest -1 with compassion. Engage with the submitter. If you don't know them, take a look at how hard it would be to write a test for the behavior and give pointers to the exact test suite that would need to be changed, or suggest a new test suite and point at a good example to copy. So, with all of this, let's make sure we don't forget to first appreciate the effort that went into submitting the patch that lacks tests. I'm not going to claim that I've always practiced "-1 with compassion", so thanks for reminding us all that we're not just reviewing code, we are having a dialog with real live people. I think this is the key thing here, thanks for bringing this up Clint. At the end of the day, patches are submitted by real people. If we want to grow the committer base and help people to become better reviewers, taking the time to show them the ropes is part of the game. I'd argue that is in fact part of what being a core is about in fact. When in doubt, I tend to err on the side of "no score with comments". It's not ideal for every situation, but I like to think it gets my point across without making it sound like I disapprove of the change itself. If my suggestions are ignored completely, I've always got the -1 in my back pocket to press the issue. :-) -Ben ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] When is it okay for submitters to say 'I don't want to add tests' ?
On 10/31/2013 06:04 AM, Rosa, Andrea (HP Cloud Services) wrote: A - there is no test suite at all, adding one in unreasonable B - this thing cannot be tested in this context (e.g. functional tests are defined in a different tree) C - this particular thing is very hard to test >> D - testing this won't offer benefit In my opinion C instead of being an acceptable reason for not having tests is a symptom of one of the two things: 1) F => Submitter doesn't know how to write tests, in this case someone else can help with suggestions 2) The code we are trying to test is too complicated so it's time to refactor it And about D, In my opinion tests always offer benefits, like code coverage or helping in understanding the code. I think there are actually cases where C is valid. It's difficult to test certain kinds of race conditions, for example, unless you have very low-level hooks into the guts of the system in order to force the desired conditions to reliably occur at exactly the right time. Chris ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] When is it okay for submitters to say 'I don't want to add tests' ?
Hi all, As a newbie of the community, I'm not familiar with unittest and how to use it here. I've learned that Jenkins runs tests everytime we submit some code. But how to write the test and what is a 'good test' and a 'bad test'? I saw some commits in gerrit but am unable to say if the written test is enough to judge the code, since it is the author of the code who writes the test. Is there a framework to follow or some rules/pratices to respect? Do you have some links to help me out? Thanks, Toan - Original Message - From: "Kyle Mestery (kmestery)" To: "OpenStack Development Mailing List (not for usage questions)" Sent: Thursday, October 31, 2013 3:05:27 PM Subject: Re: [openstack-dev] When is it okay for submitters to say 'I don't want to add tests' ? On Oct 31, 2013, at 8:56 AM, Clint Byrum wrote: > Excerpts from Mark McLoughlin's message of 2013-10-31 06:30:32 -0700: >> On Thu, 2013-10-31 at 15:37 +1300, Robert Collins wrote: >>> This is a bit of a social norms thread >>> >>> I've been consistently asking for tests in reviews for a while now, >>> and I get the occasional push-back. I think this falls into a few >>> broad camps: >>> >>> A - there is no test suite at all, adding one in unreasonable >>> B - this thing cannot be tested in this context (e.g. functional tests >>> are defined in a different tree) >>> C - this particular thing is very hard to test >>> D - testing this won't offer benefit >>> E - other things like this in the project don't have tests >>> F - submitter doesn't know how to write tests >>> G - submitter doesn't have time to write tests >> >> Nice breakdown. >> >>> Now, of these, I think it's fine not add tests in cases A, B, C in >>> combination with D, and D. >>> >>> I don't think E, F or G are sufficient reasons to merge something >>> without tests, when reviewers are asking for them. G in the special >>> case that the project really wants the patch landed - but then I'd >>> expect reviewers to not ask for tests or to volunteer that they might >>> be optional. >> >> I totally agree with the sentiment but, especially when it's a newcomer >> to the project, I try to put myself in the shoes of the patch submitter >> and double-check whether what we're asking is reasonable. >> > > Even with a long time contributor, empathy is an important part of > constructing reviews. We could make more robotic things that review for > test coverage, but we haven't, because this is a gray area. The role of > a reviewer isn't just get patches merged and stop defects. It is also to > grow the other developers. > >> For example, if someone shows up to Nova with their first OpenStack >> contribution, it fixes something which is unquestionably a bug - think >> typo like "raise NotFund('foo')" - and testing this code patch requires >> more than adding a simple new scenario to existing tests ... >> > > This goes back to my recent suggestion to help the person not with a -1 > or a +2, but with an additional patch that fixes it. > >> That, for me, is an example of "-1, we need a test! untested code is >> broken!" is really shooting the messenger, not valuing the newcomers >> contribution and risks turning that person off the project forever. >> Reviewers being overly aggressive about this where the project doesn't >> have full test coverage to begin with really makes us seem unwelcoming. >> >> In cases like that, I'd be of a mind to go "+2 Awesome! Thanks for >> catching this! It would be great to have a unit test for this, but it's >> clear the current code is broken so I'm fine with merging the fix >> without a test". You could say it's now the reviewers responsibility to >> merge a test, but if that requirement then turns off reviewers even >> reviewing such a patch, then that doesn't help either. >> > > I understand entirely why you choose this, and I think that is fine. I, > however, see this as a massive opportunity to teach. That code was only > broken because it was allowed it to be merged without tests. By letting > that situation continue, we only fix it today. The next major refactoring > has a high chance now of breaking that part of the code again. > > So, rather than +2, I suggest -1 with compassion. Engage with the > submitter. If you don't know them, take a look at how hard it would be > to write a test for the behavior
Re: [openstack-dev] When is it okay for submitters to say 'I don't want to add tests' ?
On Oct 31, 2013, at 8:56 AM, Clint Byrum wrote: > Excerpts from Mark McLoughlin's message of 2013-10-31 06:30:32 -0700: >> On Thu, 2013-10-31 at 15:37 +1300, Robert Collins wrote: >>> This is a bit of a social norms thread >>> >>> I've been consistently asking for tests in reviews for a while now, >>> and I get the occasional push-back. I think this falls into a few >>> broad camps: >>> >>> A - there is no test suite at all, adding one in unreasonable >>> B - this thing cannot be tested in this context (e.g. functional tests >>> are defined in a different tree) >>> C - this particular thing is very hard to test >>> D - testing this won't offer benefit >>> E - other things like this in the project don't have tests >>> F - submitter doesn't know how to write tests >>> G - submitter doesn't have time to write tests >> >> Nice breakdown. >> >>> Now, of these, I think it's fine not add tests in cases A, B, C in >>> combination with D, and D. >>> >>> I don't think E, F or G are sufficient reasons to merge something >>> without tests, when reviewers are asking for them. G in the special >>> case that the project really wants the patch landed - but then I'd >>> expect reviewers to not ask for tests or to volunteer that they might >>> be optional. >> >> I totally agree with the sentiment but, especially when it's a newcomer >> to the project, I try to put myself in the shoes of the patch submitter >> and double-check whether what we're asking is reasonable. >> > > Even with a long time contributor, empathy is an important part of > constructing reviews. We could make more robotic things that review for > test coverage, but we haven't, because this is a gray area. The role of > a reviewer isn't just get patches merged and stop defects. It is also to > grow the other developers. > >> For example, if someone shows up to Nova with their first OpenStack >> contribution, it fixes something which is unquestionably a bug - think >> typo like "raise NotFund('foo')" - and testing this code patch requires >> more than adding a simple new scenario to existing tests ... >> > > This goes back to my recent suggestion to help the person not with a -1 > or a +2, but with an additional patch that fixes it. > >> That, for me, is an example of "-1, we need a test! untested code is >> broken!" is really shooting the messenger, not valuing the newcomers >> contribution and risks turning that person off the project forever. >> Reviewers being overly aggressive about this where the project doesn't >> have full test coverage to begin with really makes us seem unwelcoming. >> >> In cases like that, I'd be of a mind to go "+2 Awesome! Thanks for >> catching this! It would be great to have a unit test for this, but it's >> clear the current code is broken so I'm fine with merging the fix >> without a test". You could say it's now the reviewers responsibility to >> merge a test, but if that requirement then turns off reviewers even >> reviewing such a patch, then that doesn't help either. >> > > I understand entirely why you choose this, and I think that is fine. I, > however, see this as a massive opportunity to teach. That code was only > broken because it was allowed it to be merged without tests. By letting > that situation continue, we only fix it today. The next major refactoring > has a high chance now of breaking that part of the code again. > > So, rather than +2, I suggest -1 with compassion. Engage with the > submitter. If you don't know them, take a look at how hard it would be > to write a test for the behavior and give pointers to the exact test > suite that would need to be changed, or suggest a new test suite and > point at a good example to copy. > >> So, with all of this, let's make sure we don't forget to first >> appreciate the effort that went into submitting the patch that lacks >> tests. >> > > I'm not going to claim that I've always practiced "-1 with compassion", > so thanks for reminding us all that we're not just reviewing code, we > are having a dialog with real live people. > I think this is the key thing here, thanks for bringing this up Clint. At the end of the day, patches are submitted by real people. If we want to grow the committer base and help people to become better reviewers, taking the time to show them the ropes is part of the game. I'd argue that is in fact part of what being a core is about in fact. Thanks, Kyle > ___ > OpenStack-dev mailing list > OpenStack-dev@lists.openstack.org > http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] When is it okay for submitters to say 'I don't want to add tests' ?
Excerpts from Mark McLoughlin's message of 2013-10-31 06:30:32 -0700: > On Thu, 2013-10-31 at 15:37 +1300, Robert Collins wrote: > > This is a bit of a social norms thread > > > > I've been consistently asking for tests in reviews for a while now, > > and I get the occasional push-back. I think this falls into a few > > broad camps: > > > > A - there is no test suite at all, adding one in unreasonable > > B - this thing cannot be tested in this context (e.g. functional tests > > are defined in a different tree) > > C - this particular thing is very hard to test > > D - testing this won't offer benefit > > E - other things like this in the project don't have tests > > F - submitter doesn't know how to write tests > > G - submitter doesn't have time to write tests > > Nice breakdown. > > > Now, of these, I think it's fine not add tests in cases A, B, C in > > combination with D, and D. > > > > I don't think E, F or G are sufficient reasons to merge something > > without tests, when reviewers are asking for them. G in the special > > case that the project really wants the patch landed - but then I'd > > expect reviewers to not ask for tests or to volunteer that they might > > be optional. > > I totally agree with the sentiment but, especially when it's a newcomer > to the project, I try to put myself in the shoes of the patch submitter > and double-check whether what we're asking is reasonable. > Even with a long time contributor, empathy is an important part of constructing reviews. We could make more robotic things that review for test coverage, but we haven't, because this is a gray area. The role of a reviewer isn't just get patches merged and stop defects. It is also to grow the other developers. > For example, if someone shows up to Nova with their first OpenStack > contribution, it fixes something which is unquestionably a bug - think > typo like "raise NotFund('foo')" - and testing this code patch requires > more than adding a simple new scenario to existing tests ... > This goes back to my recent suggestion to help the person not with a -1 or a +2, but with an additional patch that fixes it. > That, for me, is an example of "-1, we need a test! untested code is > broken!" is really shooting the messenger, not valuing the newcomers > contribution and risks turning that person off the project forever. > Reviewers being overly aggressive about this where the project doesn't > have full test coverage to begin with really makes us seem unwelcoming. > > In cases like that, I'd be of a mind to go "+2 Awesome! Thanks for > catching this! It would be great to have a unit test for this, but it's > clear the current code is broken so I'm fine with merging the fix > without a test". You could say it's now the reviewers responsibility to > merge a test, but if that requirement then turns off reviewers even > reviewing such a patch, then that doesn't help either. > I understand entirely why you choose this, and I think that is fine. I, however, see this as a massive opportunity to teach. That code was only broken because it was allowed it to be merged without tests. By letting that situation continue, we only fix it today. The next major refactoring has a high chance now of breaking that part of the code again. So, rather than +2, I suggest -1 with compassion. Engage with the submitter. If you don't know them, take a look at how hard it would be to write a test for the behavior and give pointers to the exact test suite that would need to be changed, or suggest a new test suite and point at a good example to copy. > So, with all of this, let's make sure we don't forget to first > appreciate the effort that went into submitting the patch that lacks > tests. > I'm not going to claim that I've always practiced "-1 with compassion", so thanks for reminding us all that we're not just reviewing code, we are having a dialog with real live people. ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] When is it okay for submitters to say 'I don't want to add tests' ?
On Thu, 2013-10-31 at 15:37 +1300, Robert Collins wrote: > This is a bit of a social norms thread > > I've been consistently asking for tests in reviews for a while now, > and I get the occasional push-back. I think this falls into a few > broad camps: > > A - there is no test suite at all, adding one in unreasonable > B - this thing cannot be tested in this context (e.g. functional tests > are defined in a different tree) > C - this particular thing is very hard to test > D - testing this won't offer benefit > E - other things like this in the project don't have tests > F - submitter doesn't know how to write tests > G - submitter doesn't have time to write tests Nice breakdown. > Now, of these, I think it's fine not add tests in cases A, B, C in > combination with D, and D. > > I don't think E, F or G are sufficient reasons to merge something > without tests, when reviewers are asking for them. G in the special > case that the project really wants the patch landed - but then I'd > expect reviewers to not ask for tests or to volunteer that they might > be optional. I totally agree with the sentiment but, especially when it's a newcomer to the project, I try to put myself in the shoes of the patch submitter and double-check whether what we're asking is reasonable. For example, if someone shows up to Nova with their first OpenStack contribution, it fixes something which is unquestionably a bug - think typo like "raise NotFund('foo')" - and testing this code patch requires more than adding a simple new scenario to existing tests ... That, for me, is an example of "-1, we need a test! untested code is broken!" is really shooting the messenger, not valuing the newcomers contribution and risks turning that person off the project forever. Reviewers being overly aggressive about this where the project doesn't have full test coverage to begin with really makes us seem unwelcoming. In cases like that, I'd be of a mind to go "+2 Awesome! Thanks for catching this! It would be great to have a unit test for this, but it's clear the current code is broken so I'm fine with merging the fix without a test". You could say it's now the reviewers responsibility to merge a test, but if that requirement then turns off reviewers even reviewing such a patch, then that doesn't help either. So, with all of this, let's make sure we don't forget to first appreciate the effort that went into submitting the patch that lacks tests. Mark. ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] When is it okay for submitters to say 'I don't want to add tests' ?
On 10/30/2013 10:37 PM, Robert Collins wrote: This is a bit of a social norms thread I've been consistently asking for tests in reviews for a while now, and I get the occasional push-back. I think this falls into a few broad camps: A - there is no test suite at all, adding one in unreasonable B - this thing cannot be tested in this context (e.g. functional tests are defined in a different tree) C - this particular thing is very hard to test D - testing this won't offer benefit E - other things like this in the project don't have tests F - submitter doesn't know how to write tests G - submitter doesn't have time to write tests Now, of these, I think it's fine not add tests in cases A, B, C in combination with D, and D. I don't think E, F or G are sufficient reasons to merge something without tests, when reviewers are asking for them. G in the special case that the project really wants the patch landed - but then I'd expect reviewers to not ask for tests or to volunteer that they might be optional. Now, if I'm wrong, and folk have different norms about when to accept 'reason X not to write tests' as a response from the submitter - please let me know! First - providing tests with code is a basic design tenant of OpenStack - https://wiki.openstack.org/wiki/BasicDesignTenets (I realize we don't highlight that as much as we used to, but it really is baked in culture to people that have been here for a while). This is pretty close to my thinking. I like to challenge people on C. There was a pretty good case of it late in grizzly or havana where I was reviewing some nova virt code and the sumbitter claimed it was too hard to test. A week later danpb wrote a 100 line patch that provided the entire test framework for the case. So "C" was a pretty BS answer. Adding tests and test frameworks are a way of paying it forward in the project. Much like doing reviews, adding tests is a way to save other people on the project time. Good tests pay for themselves very quickly in time lost on the project. So I generally find "I don't want to add tests" the same kind of selfishness as "why should I have to review code?". A good example of the kind of savings we get, over the summer when were were regularlly breaking on requirements mismatch (and on the update script modifying things incorrectly), I went in and added a testing framework to requirements repo, so we knew all the parts were doing expected things. Without that we'd still be fighting these things on a weekly basis, now it's a solved problem, and we can go fight the next fire. This was actually as C class problem (which is why no one else had done it yet), and required building new devstack job to complete. However the benefits were really high on it. -Sean -- Sean Dague http://dague.net ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] When is it okay for submitters to say 'I don't want to add tests' ?
Hi all, >This is a bit of a social norms thread > >I've been consistently asking for tests in reviews for a while now, and I get >the >occasional push-back. I think this falls into a few broad camps: > >A - there is no test suite at all, adding one in unreasonable B - this thing >cannot >be tested in this context (e.g. functional tests are defined in a different >tree) >C - this particular thing is very hard to test D - testing this won't offer >benefit E >- other things like this in the project don't have tests F - submitter doesn't >know how to write tests G - submitter doesn't have time to write tests > >Now, of these, I think it's fine not add tests in cases A, B, C in combination >with D, and D. In my opinion C instead of being an acceptable reason for not having tests is a symptom of one of the two things: 1) F => Submitter doesn't know how to write tests, in this case someone else can help with suggestions 2) The code we are trying to test is too complicated so it's time to refactor it And about D, In my opinion tests always offer benefits, like code coverage or helping in understanding the code. What do you think? Regards -- Andrea Rosa ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] When is it okay for submitters to say 'I don't want to add tests' ?
+1 I think C always means something is not architected/designed correctly or invokes the 'code smell'[1] to me. If its to hard to test, then it will be to hard to change, which will in the long term be a liability. Better to avoid the liability upfront by just fixing the cause of the 'smell' in the first place :-) 1. http://en.wikipedia.org/wiki/Code_smell On 10/30/13 7:51 PM, "Morgan Fainberg" wrote: >On Wed, Oct 30, 2013 at 7:37 PM, Robert Collins > wrote: >> This is a bit of a social norms thread >> >> I've been consistently asking for tests in reviews for a while now, >> and I get the occasional push-back. I think this falls into a few >> broad camps: >> >> A - there is no test suite at all, adding one in unreasonable >> B - this thing cannot be tested in this context (e.g. functional tests >> are defined in a different tree) >> C - this particular thing is very hard to test >> D - testing this won't offer benefit >> E - other things like this in the project don't have tests >> F - submitter doesn't know how to write tests >> G - submitter doesn't have time to write tests >> >> Now, of these, I think it's fine not add tests in cases A, B, C in >> combination with D, and D. >> > >I think that C is not a really valid case to allow no tests (there are >always exceptions), I strongly believe that we can collaborate >(perhaps include some people who are better at test writing) to build >the tests and add them as a co-author in the commit message (or at the >very least have the test patchset be dependent on the main patchset. >"Hard to test" is too similar to F. > >Just my general feelings on the topic. > >Cheers, >Morgan Fainberg > >___ >OpenStack-dev mailing list >OpenStack-dev@lists.openstack.org >http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] When is it okay for submitters to say 'I don't want to add tests' ?
> When is it okay for submitters to say 'I don't want to add tests' ? When they don't want their patch merged :-) But more seriously On Thu, Oct 31, 2013 at 1:07 PM, Robert Collins wrote: > This is a bit of a social norms thread > > I've been consistently asking for tests in reviews for a while now, > and I get the occasional push-back. I think this falls into a few > broad camps: > > A - there is no test suite at all, adding one in unreasonable > Do you mean no test suite at all for the specific project or the feature they are modifying? For the latter I think its fine to require that they start one for the feature even if it just tests the behavior they just modified. > B - this thing cannot be tested in this context (e.g. functional tests > are defined in a different tree) > C - this particular thing is very hard to test > Yea I have a lot of sympathy for this one. I run into this occasionally myself where it can be really difficult to cleanly, yet still vaguely realistically inject an error. I'm sort of stuck on one at the moment and I've definitely spent a lot more time thinking about how to write an adequate test than the fix itself. > D - testing this won't offer benefit > The test will at least always act as a check that the specific behaviour doesn't change accidentally in the future won't it (in the context of bug fixes) ? > E - other things like this in the project don't have tests > F - submitter doesn't know how to write tests > G - submitter doesn't have time to write tests > > There is perhaps also: H - a test in another project (say tempest) picks up this failure already. and H is in combination with say C. I'd have a lot of sympathy for that situation. Now, of these, I think it's fine not add tests in cases A, B, C in > combination with D, and D. > > I don't think E, F or G are sufficient reasons to merge something > without tests, when reviewers are asking for them. G in the special > case that the project really wants the patch landed - but then I'd > expect reviewers to not ask for tests or to volunteer that they might > be optional. > I agree with you on E, F and G. But in those circumstances we should be offering those people help in writing tests, especially if they're new. Chris ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] When is it okay for submitters to say 'I don't want to add tests' ?
On Wed, Oct 30, 2013 at 7:37 PM, Robert Collins wrote: > This is a bit of a social norms thread > > I've been consistently asking for tests in reviews for a while now, > and I get the occasional push-back. I think this falls into a few > broad camps: > > A - there is no test suite at all, adding one in unreasonable > B - this thing cannot be tested in this context (e.g. functional tests > are defined in a different tree) > C - this particular thing is very hard to test > D - testing this won't offer benefit > E - other things like this in the project don't have tests > F - submitter doesn't know how to write tests > G - submitter doesn't have time to write tests > > Now, of these, I think it's fine not add tests in cases A, B, C in > combination with D, and D. > I think that C is not a really valid case to allow no tests (there are always exceptions), I strongly believe that we can collaborate (perhaps include some people who are better at test writing) to build the tests and add them as a co-author in the commit message (or at the very least have the test patchset be dependent on the main patchset. "Hard to test" is too similar to F. Just my general feelings on the topic. Cheers, Morgan Fainberg ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
[openstack-dev] When is it okay for submitters to say 'I don't want to add tests' ?
This is a bit of a social norms thread I've been consistently asking for tests in reviews for a while now, and I get the occasional push-back. I think this falls into a few broad camps: A - there is no test suite at all, adding one in unreasonable B - this thing cannot be tested in this context (e.g. functional tests are defined in a different tree) C - this particular thing is very hard to test D - testing this won't offer benefit E - other things like this in the project don't have tests F - submitter doesn't know how to write tests G - submitter doesn't have time to write tests Now, of these, I think it's fine not add tests in cases A, B, C in combination with D, and D. I don't think E, F or G are sufficient reasons to merge something without tests, when reviewers are asking for them. G in the special case that the project really wants the patch landed - but then I'd expect reviewers to not ask for tests or to volunteer that they might be optional. Now, if I'm wrong, and folk have different norms about when to accept 'reason X not to write tests' as a response from the submitter - please let me know! -Rob -- Robert Collins Distinguished Technologist HP Converged Cloud ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev