Open Stack

On Aug 13, 2014, at 11:11 AM, Kevin Benton <blak111 at gmail.com> wrote:

> Is the pylint static analysis that caught that error prone to false
> positives? If not, I agree that it would be really nice if that were made
> part of the tox check so these don't have to be fixed after the fact.
> 
> To me that particular patch seems like one that should be accompanied with
> a unit test because it's a failure case with cleanup code that isn't being
> touched by the unit tests.

+1

As a general rule I would like to see test addition included with any fix targeting a bug that was merged due to a lack of coverage.


m.


> On Aug 13, 2014 8:34 AM, "Armando M." <armamig at gmail.com> wrote:
> 
>> I am gonna add more color to this story by posting my replies on review
>> [1]:
>> 
>> Hi Angus,
>> 
>> You touched on a number of points. Let me try to give you an answer to all
>> of them.
>> 
>>>> (I'll create a bug report too. I still haven't worked out which class
>> of changes need an accompanying bug report and which don't.)
>> 
>> The long story can be read below:
>> 
>> https://wiki.openstack.org/wiki/BugFilingRecommendations
>> 
>> https://wiki.openstack.org/wiki/GitCommitMessages
>> 
>> IMO, there's a grey area for some of the issues you found, but when I am
>> faced with a bug, I tend to answer myself? Would a bug report be useful to
>> someone else? The author of the code? A consumer of the code? Not everyone
>> follow the core review system all the time, whereas Launchpad is pretty
>> much the tool everyone uses to stay abreast with the OpenStack release
>> cycle. Obviously if you're fixing a grammar nit, or filing a cosmetic
>> change that has no functional impact then I warrant the lack of a test, but
>> in this case you're fixing a genuine error: let's say we want to backport
>> this to icehouse, how else would we make the release manager of that?
>> He/she is looking at Launchpad.
>> 
>>>> I can add a unittest for this particular code path, but it would only
>> check this particular short segment of code, would need to be maintained as
>> the code changes, and wouldn't catch another occurrence somewhere else.
>> This seems an unsatisfying return on the additional work :(
>> 
>> You're looking at this from the wrong perspective. This is not about
>> ensuring that other code paths are valid, but that this code path stays
>> valid over time, ensuring that the code path is exercised and that no other
>> regression of any kind creep in. The reason why this error was introduced
>> in the first place is because the code wasn't tested when it should have.
>> If you don't get that this mechanical effort of fixing errors by static
>> analysis is kind of ineffective, which leads me to the last point....
>> 
>>>> I actually found this via static analysis using pylint - and my
>> question is: should I create some sort of pylint unittest that tries to
>> catch this class of problem across the entire codebase? [...]
>> 
>> I value what you're doing, however I would see even more value if we
>> prevented these types of errors from occurring in the first place via
>> automation. You run pylint today, but what about tomorrow, or a week from
>> now? Are you gonna be filing pylint fixes for ever? We might be better off
>> automating the check and catch these types of errors before they land in
>> the tree. This means that the work you are doing it two-pronged: a)
>> automate the detection of some failures by hooking this into tox.ini via
>> HACKING/pep8 or equivalent mechanism and b) file all the fixes that require
>> these validation tests to pass; c) everyone is happy, or at least they
>> should be.
>> 
>> I'd welcome to explore a better strategy to ensure a better quality of the
>> code base, without some degree of automation, nothing will stop these
>> conversation from happening again.
>> 
>> Cheers,
>> 
>> Armando
>> 
>> [1] https://review.openstack.org/#/c/113777/
>> 
>> 
>> On 13 August 2014 03:02, Ihar Hrachyshka <ihrachys at redhat.com> wrote:
>> 
>>> -----BEGIN PGP SIGNED MESSAGE-----
>>> Hash: SHA512
>>> 
>>> On 13/08/14 09:28, Angus Lees wrote:
>>>> I'm doing various small cleanup changes as I explore the neutron
>>>> codebase. Some of these cleanups are to fix actual bugs discovered
>>>> in the code.  Almost all of them are tiny and "obviously correct".
>>>> 
>>>> A recurring reviewer comment is that the change should have had an
>>>> accompanying bug report and that they would rather that change was
>>>> not submitted without one (or at least, they've -1'ed my change).
>>>> 
>>>> I often didn't discover these issues by encountering an actual
>>>> production issue so I'm unsure what to include in the bug report
>>>> other than basically a copy of the change description.  I also
>>>> haven't worked out the pattern yet of which changes should have a
>>>> bug and which don't need one.
>>>> 
>>>> There's a section describing blueprints in NeutronDevelopment but
>>>> nothing on bugs.  It would be great if someone who understands the
>>>> nuances here could add some words on when to file bugs: Which type
>>>> of changes should have accompanying bug reports? What is the
>>>> purpose of that bug, and what should it contain?
>>>> 
>>> 
>>> It was discussed before at:
>>> http://lists.openstack.org/pipermail/openstack-dev/2014-May/035789.html
>>> 
>>> /Ihar
>>> -----BEGIN PGP SIGNATURE-----
>>> Version: GnuPG/MacGPG2 v2.0.22 (Darwin)
>>> 
>>> iQEcBAEBCgAGBQJT6zfOAAoJEC5aWaUY1u570wQIAMpoXIK/p5invp+GW0aMMUK0
>>> C/MR6WIJ83e6e2tOVUrxheK6bncVvidOI4EWGW1xzP1sg9q+8Hs1TNyKHXhJAb+I
>>> c435MMHWsDwj6p1OeDxHnSOVMthcGH96sgRa1+CIk6+oktDF3IMmiOPRkxdpqWCZ
>>> 7TkV75mryehrTNwAkVPfpWG3OhWO44d5lLnJFCIMCuOw2NHzyLIOoGQAlWNQpy4V
>>> a869s00WO37GEed6A5Zizc9K/05/6kpDIQVim37tw91JcZ69VelUlZ1THx+RTd33
>>> 92r87APm3fC/LioKN3fq1UUo2c94Vzl3gYPFVl8ZateQNMKB7ONMBePOfWR9H1k=
>>> =wCJQ
>>> -----END PGP SIGNATURE-----
>>> 
>>> _______________________________________________
>>> OpenStack-dev mailing list
>>> OpenStack-dev at lists.openstack.org
>>> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>>> 
>> 
>> 
>> _______________________________________________
>> OpenStack-dev mailing list
>> OpenStack-dev at lists.openstack.org
>> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>> 
>> 
> _______________________________________________
> OpenStack-dev mailing list
> OpenStack-dev at lists.openstack.org
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev