2018年5月30日(水) 23:36 Carsten Haitzler <ras...@rasterman.com>:
> On Wed, 30 May 2018 09:06:09 -0500 Derek Foreman <der...@osg.samsung.com>
> said:
>
> > 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%
>
> actually... it wasn't one from you. fyi. :)
>
> > However, review and testing aren't strictly the same thing. Reviewers
> > shouldn't have to ensure code compiles without warnings - compilers are
>
> no - not compile warnings. i'm talking compile ERRORS. like syntax error
> and
> compiler barfs. it means the patch was never compiled let-alone run.
> sometimes
> it's hard to run, and sometimes compiling means the change is deep in an
> ifdef
> section that may be hard to enable, but when it's in non-ifdef'd code that
> always compiles no matter what and it's a syntax error that stops the
> compiler... then i consider this "bad review" where not even a compilation
> was
> done by the reviewer. it's an automated check to see it compiles as a
> human is
> going to miss the missing ) if there are several of them like:
>
> if (func(x)) {
>
> if the last ) was missing a human is going to easily miss it, but not a
> compiler. :) so my point is that simple basic things like this i see are
> being
> missed in review. :(
>
> > 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).
>
> hmm i expect the reviewer to also compile it and if possible run it. if
> needed
> compile against it too.
I agree with Derek that testing and reviewing are different and it could be
up for discussion to decide if a mandatory testing phase should or
shouldn't be in the review process.
The reviewee should of course test it... but it should be up to the
reviewer to decide if testing on his side is necessary. You need a balance
and also you need your own time to do your own thing.
I think it takes too much time to test everything and you should consider
that an old efl developer is at least testing its patches. of course shit
always happens :)
------------------------------------------------------------------------------
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
