On Aug 13, 2014, at 11:11 AM, Kevin Benton <blak...@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." <arma...@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 <ihrac...@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@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 >> >> > _______________________________________________ > 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