On 2018-05-29 12:08 PM, Carsten Haitzler (The Rasterman) wrote: > On Tue, 29 May 2018 10:13:33 -0400 Mike Blumenkrantz > <michael.blumenkra...@gmail.com> said: > >> While it is true that a mail was sent by JackDanielz related to this patch, >> the elapsed time between that mail and this revert is roughly 1.5 hours on >> a Saturday afternoon during a federal holiday weekend. If inactivity from a >> patch author is going to be used as reasoning for a revert, I think that >> the time period given should be a reasonable one. > > it's the severity of the break. a totally blank screen. impossible to recover > from without going to a text console and finding a better working revision. > when > a break is severe enough then a revert asap is what needs doing with people > already finding it and complaining after an hour or so. > >> As stated in the original thread, testing for this patch occurred on a >> number of machines, all with a similar quirk: modesetting occurs at the >> start of every session. This happened to work around the issue exhibited in >> some sessions. It's a bit unlucky, but that's the sort of result that can >> be expected when there is no unit testing for the component being modified. > > see below... > >> Questioning the review process in your case seems like a bit of a slippery >> slope considering that the patch was submitted over 3 weeks ago with you as >> a reviewer, and it was mentioned more than once in another ticket that you >> were active in at that time. > > i didn't notice it... but my point is that it's quite severe and review didn't > find it. but a review should involve trying the code out and giving it a test > for possible failure modes. what kind of review is going on? is it just a scan > "oh that looks good to me?"... if so what is the point if it's not catching > problems by at least trying it out (decently)? this also goes for just > compiling the code. > > whilst i was checking some older revisions i found a commit that went through > review, was approved AND pushed... and it didn't compile because it was > missing a ) - not even in an ifdef for some specific os or display system. it > never compiled. sure. a few commits after was another reviewed patch that > fixed > this. what kind of review is going on that lets basic stuff like this through > (you and cedric were not involved with this one)? this was a few weeks ago.
This happened to me recently, so you're probably citing my patches here. I made a last minute "trivial" change to a patch set and screwed it up. It slipped past review, and it landed that way. My bad. 100% However, review and testing aren't strictly the same thing. Reviewers shouldn't have to ensure code compiles without warnings - compilers are excellent tools for that, and the author of the code is expected to have run one. A reviewer is supposed to look for high level issues and goodness of fit (does this match project style, should this code be in the project at all). There are far worse problems than build breaks. In general a build break is fairly easy to resolve, and usually the person responsible will fix it as quickly as possible (though there of course are times when that is not the case). EFL contains a large amount of idiosyncratic code, and quite frequently fixing an obvious bug will unmask multiple other bugs. At that point EFL blame culture kicks in and the author is accused of being a "borker", that their patches are terrible, that they have destroyed [key functionality] etc etc. The point of code review isn't to stop 100% of build breaks - it's to reduce the chance that unmaintainable flaky code lands by having peers review it to provide opinions. Perhaps one of the reviewers will remember some savage hack in another part of the tree that's going to fall over now. "Basic testing" in E/EFL is essentially undefined as there are so many different engines, gadgets, possible monitor layouts, target GPUs, etc etc etc, that something that manifests in 0 seconds for you might never happen at all for me unless I remember to fiddle with options, turn on things I don't usually use, or whatever. Without more comprehensive automated testing every developer becomes responsible for an intractable amount of "basic testing". tl;dr. yup, I broke the build for a couple of hours a few weeks ago with a "reviewed" patch - I'm really sorry about that. To use that as proof that review isn't worthwhile or not rigorous seems very shortsighted. Thanks, Derek >> Had the commit log for this revert been only "this patch causes this >> regression" with a simple description of the issue, it's unlikely that this >> mail would exist. Instead, however, it seems like this revert was being >> used to bash both patch review and testing--something which makes little >> sense considering that you declined to attempt either when it was >> explicitly requested of you. I don't think it's fair for you to >> (indirectly) criticize Cedric for trying to do the work that you did not do. > > the person reviewing and approving takes on the responsibility to ensure > quality. me being involved or not is not relevant to the quality of the review > (testing) that was done. > > it's a general thing - test things. i find commits that break simple things > that should have been caught with fairly basic testing fairly often. when i do > i make noise about it to remind people to improve their testing. > >> I can understand your mindset in writing this commit log and related mail, >> but I don't think statements like these are beneficial to us as a community >> or the project as a whole. > > well you explained what about testing made it not trigger, though that would > only explain i believe starting e afresh on a new xserver for example, and > never > restarting it... or never testing e in x... > > it triggered instantly on both my desktop and 2 laptops (screen resolutions > did > not need a reconfigure on all 3... and one is multi-screen). any restart of e > (ctrl+alt+end) should bring this out. on restart the screen is already in the > final config state and thus no change will happen thus no configure event. > > i restart e when i'm testing things at least twice (once to try the newly > compiled code for the first time, and then again to check shutdown and > re-exec work). i still maintain that testing here was poor. a restart (in x) > would show the issue right away (no reconfigure of screen). since i believe > this is the case, my take is that testing was inadequate for sure. it appeared > immediately on 3 of my machines after rebuilding and ctrl+alt+end immediately. > no exceptions. > > review should involve testing the code in question. decently. so ok - it was > run... but not enough. testing needs to be far better. totally "it doesn't > work > at all" kinds of changes need to not slip through if at all possible. > >> On Sat, May 26, 2018 at 4:01 PM Carsten Haitzler <ras...@rasterman.com> >> wrote: >> >>> raster pushed a commit to branch master. >>> >>> >>> http://git.enlightenment.org/core/efl.git/commit/?id=4715c099ecb1e9a5c7de5dab883560e198704c72 >>> >>> commit 4715c099ecb1e9a5c7de5dab883560e198704c72 >>> Author: Carsten Haitzler (Rasterman) <ras...@rasterman.com> >>> Date: Sun May 27 04:52:03 2018 +0900 >>> >>> Revert "ecore-evas-x: set draw_block until the window receives a >>> configure event" >>> >>> This reverts commit 7b80038fa7b54cff27b463382283211727aaf104. >>> >>> JackDanielz asked nicely, but this hasn't been reverted. As this >>> totally breaks enlightenment (it's black) and this happens on >>> everythng I've tested (1 laptop, desktop and Xephyr) I'm calling this >>> patch a dud. >>> >>> Now... what kind of review is going on here? This hasn't been tested. >>> What kind of review doesn't build + run things? >>> >>> for the reasons of poor review and massive horribler fully complete >>> desktop like breakage ... this gets reverted as master should not be >>> broken like this. >>> --- >>> src/modules/ecore_evas/engines/x/ecore_evas_x.c | 27 >>> ++----------------------- >>> 1 file changed, 2 insertions(+), 25 deletions(-) >>> >>> diff --git a/src/modules/ecore_evas/engines/x/ecore_evas_x.c >>> b/src/modules/ecore_evas/engines/x/ecore_evas_x.c >>> index 47baeff5ff..3d88e9aae3 100644 >>> --- a/src/modules/ecore_evas/engines/x/ecore_evas_x.c >>> +++ b/src/modules/ecore_evas/engines/x/ecore_evas_x.c >>> >>> >> ------------------------------------------------------------------------------ >> Check out the vibrant tech community on one of the world's most >> engaging tech sites, Slashdot.org! http://sdm.link/slashdot >> _______________________________________________ >> enlightenment-devel mailing list >> enlightenment-devel@lists.sourceforge.net >> https://lists.sourceforge.net/lists/listinfo/enlightenment-devel >> > > ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot _______________________________________________ enlightenment-devel mailing list enlightenment-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
