Hard to estimate scope/size of review for mozreview review requests and properly budget time for them

RESOLVED FIXED

Status

RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: bzbarsky, Assigned: smacleod, Mentored)

Tracking

Details

When someone asks me for review in bugzilla, it's easy to get a decent estimate of the time I should expect the review to take: I look at the size, in bytes, of the attached diff.  Bugzilla conveniently shows that in the attachment listing.  This is particularly important when there are multiple patches, because you can get this information without having to open them one by one.

I see nothing equivalent with mozreview.  The bugzilla attachments are all the same 40-byte standard redirect, and the actual mozreview UI gives no indication of how extensive the changes in each changeset are.

If I were doing a review every so often, on a FIFO basis, this would not matter.  But since most of my time is spent doing reviews and I have to figure out how to prioritize them and schedule them into whatever time I have available, it's pretty important to estimate up front, and quickly, whether I'm looking at a 10-minute review or a 3-hour one.  Some indication of diff size (at -U8, for consistency with everything else) on the actual bugzilla attachment would be quite helpful here.

Comment 1

3 years ago
We could add this to the MozReview table.  Would that work?  Another option would be including it in the attachment description.
(Reporter)

Comment 2

3 years ago
Adding it to the mozreview table means I have to follow a link from the bug to get the info.  It would be better than nothing, but having it directly in the attachment description would work better for me.  Best yet, do both?  ;)

Comment 3

3 years ago
Oh I meant the MozReview table *in* the bug.  But yeah, we could add it to the table in MozReview as well (which we call the commits table).  Or I can do all three, as you say. :)
(Reporter)

Comment 4

3 years ago
Oh, I see.  I don't see much point in duplicating between the mozreview table and the attachment descriptions.  Whichever of those is easier worksforme.  ;)

Comment 5

3 years ago
We can get file counts easily. I'm not sure about diff sizes or diff stats (insertions and deletions count). But we can calculate those in Mercurial land pretty easily. I even think there is a {diffstat} template that exposes it.
(Reporter)

Comment 6

3 years ago
File counts aren't very useful.  What's most useful is some measure of "how much text will I need to read?"
Product: Developer Services → MozReview
Duplicate of this bug: 1285295
Not having the file size of the diff is a clear regression over bugzilla patches. It isn't a super big problem, but it is unfortunate. Personally I'd be fine with this just being in mozreview and not bugzilla proper.

Comment 9

2 years ago
As a quick fix, Mercurial can calculate the "diff stats" when it submits changesets to MozReview and include them in the review request somehow. Perhaps in the review request description (which is a copy of the commit message today).

Slightly more effort is to store the diff stats explicitly as a structured type on the review request and add custom UI to display it. Benefits of the latter is we can expose the data easier. e.g. an IRC bot could be like "you just got a one-liner code review: should be an easy review."
(Assignee)

Comment 10

2 years ago
(In reply to Gregory Szorc [:gps] from comment #9)
> As a quick fix, Mercurial can calculate the "diff stats" when it submits
> changesets to MozReview and include them in the review request somehow.
> Perhaps in the review request description (which is a copy of the commit
> message today).
> 
> Slightly more effort is to store the diff stats explicitly as a structured
> type on the review request and add custom UI to display it. Benefits of the
> latter is we can expose the data easier. e.g. an IRC bot could be like "you
> just got a one-liner code review: should be an easy review."

Review Board has the insertion / deletion counts stored for every file change in each diff - it would be quite easy to sum these and spit them out in all the right places.
(Assignee)

Updated

2 years ago
Mentor: smacleod
(Assignee)

Comment 11

2 years ago
To fix this we'll want to do a couple things:
 a) Iterate over the filediff objects for the relevant diffset and sum the insertions / deletions.
 b) Spit out this information in the ReviewRequestSummary resource.
 c) Output this information as part of the Commits table (we're already low on space, this may be more difficult).
 d) Update the MozReview BMO extension to make use of the new data in the ReviewRequestSummary resource.
(Assignee)

Updated

2 years ago
Depends on: 1286017
(Assignee)

Updated

2 years ago
Depends on: 1286027
(Assignee)

Updated

2 years ago
Depends on: 1286030
I /think/ this is covered by the commits in bug 1286017, but we'll want the upcoming notification bots (notably IRC) to be to display the diff size in the notification. By including it, people can quickly see it is small and do a quick review if so inclined.
(Assignee)

Comment 13

2 years ago
(In reply to Gregory Szorc [:gps] from comment #12)
> I /think/ this is covered by the commits in bug 1286017
Correct, the bot should be able to hit the summary API to fetch this information. Alternatively, we *could* tack it onto the pulse messages quite easily. If that'd be better please file a bug for it.
The Pulse messages are intentionally light weight. I don't think diff size meets the criteria for being included.
bz: The number of lines added (if any) and removed (if any) are now displayed both in the Review Board commits table and in Bugzilla's MozReview table.  I realize this isn't exactly what you were looking for in comment 0, but does this satisfy your estimation needs?

Btw we can get a bit more accurate (see bug 1287551) but I think this is pretty useful on its own.
Flags: needinfo?(bzbarsky)
I'm still trying to get used to the new setup.  Hard to tell how well it works until I use it for a bit...

For one thing, I need to train myself to click on the mozreview link, not the bug link, when deciding whether to do the review.  Sadly, the bug is where a lot of the information that I need to do the review is.  :(
Flags: needinfo?(bzbarsky)
Sorry, clicking on the mozreview vs bug link from where?  Both mozreview (Review Board specifically) and Bugzilla have the same lines-changed stats.  But maybe (probably) I'm misunderstanding you.
From the bugmail.  I hadn't noticed that the bug had the stats, since they were not next to the things I normally click on to do the review (the attachment table).  Thank you for pointing those out.
No problem, it had only landed earlier that day. :)

I'm going to resolve this since I believe we've been able to meet your minimum requirements here.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED

Updated

2 years ago
Assignee: nobody → mcote

Updated

2 years ago
Assignee: mcote → smacleod
You need to log in before you can comment on or make changes to this bug.