Closed Bug 1461492 Opened 3 years ago Closed 2 years ago

Add an optional regressed-by field in bugs

Categories

(bugzilla.mozilla.org :: General, enhancement)

Product:
bugzilla.mozilla.org
Component:
General
Version:
Production
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: marco, Assigned: kohei.yoshino)

References

(Blocks 3 open bugs)

Details

Attachments

(2 files)

GitHub Pull Request
46 bytes, text/x-github-pull-request
Details | Review
GitHub Pull Request - DB schema change only
46 bytes, text/x-github-pull-request
Details | Review 
There are ways to find, given a regression bug, which bug caused the regression, but they are hardly 100% accurate.

It would be nice if developers had a way to mark which bug caused another. Currently, sometimes the "Blocking" or "Depends on" fields are used to do that, which obviously has several shortcomings.
This has been discussed before, and I'm supportive of it, but it will require documentation and a commitment by all parties to abide by the policy. 

I'd be happy to draft a policy on this and we can explore what that field would look like in use on the development BMO instance. 

-- Emma
I'm ni'ing a group of eng. managers who are in the weekly regression triage meeting for feedback on this.
Flags: needinfo?(past)
Flags: needinfo?(mreavy)
Flags: needinfo?(jmathies)
Also, this implies bugs that introduced regressions should have a 'regresses' field.
I support such a change.
Flags: needinfo?(past)
For more context:
* Stuart and myself have projects to use more and more machine learning on bugs. As part of an analysis that Calixte did last year, not having clear regression information was a big pain for this work.
* We have some toolings (as part of https://github.com/mozilla/relman-auto-nag) which helps us identify missing metadata in the bug.
This will help us to update this field.
:dylan is it possible to set up a regressed-by and regresses relationship without having to write code?

If not, I can at least set up 'regressed-by' so that we can unblock people.
Flags: needinfo?(dylan)
I can see where this would be clearer, and ease various bits of automation, at the addition of yet more complexity/fields, which is a minus.

There will be edge cases (regressed by a combination of bug X and bug Y - for example if bug Y flips a pref to enable code in bug X).  However, you can just link it to one of them.  Right now, as said, people (normally, if they remember) mark a regression as blocking the original bug that landed the code.  Which, as is said, is confusing at times (but Blocks vs Depends On is confusing in general for this case; it works better for "Bug X can't land until bug Y lands").  

If we add Regressed-By, we need to add Regresses, or we need to add any bug poked by Regressed-By as a blocker to the bug.  That would mean if Bug Y was a regression caused by Bug X, you set Regressed-By: X in bug Y, and bugzilla would also add Bug Y to the Depends On field of bug X, and add bug X to the Blocks field of Bug Y.

Note: No one will go back and set Regressed-By on older bugs, barring a few random exceptions.

IMO, of course
+1, this change will help.
Flags: needinfo?(mreavy)
Seems like a good idea if the process is simpler than hooking up blocking deps. I'd suggest making this a comma separated list type input.
Flags: needinfo?(jmathies)
(In reply to Jim Mathies [:jimm] from comment #9)
> Seems like a good idea if the process is simpler than hooking up blocking
> deps. I'd suggest making this a comma separated list type input.

I discussed this with Dylan and it should be able to implemented in the same manner as the blocks/blocked-by fields, both of which support lists of bugs.

Working with Dylan to schedule the work.
We discussed with the release management. We think that we should limit the number of bug to 1 that in field
To be more explicit, regressed-by should accept only one bug. In the vast majority of the cases, it will be case.
this looks a lot like bug 490926.

it's been a while, but iirc bugzilla already supports custom fields that hold a single relationship bug id.

the blockers on bug 490926 were to support multiple bugs.
Duplicate of this bug: 490926
Okay, if we're all clear on this supporting one bug, then I will to add this to my list of plans to draft so we can enable it. 

Thank you.
Will update this bug after I conduct an experiment.
Assignee: nobody → dylan
Flags: needinfo?(dylan)
Any news from your experiment?
Flags: needinfo?(dylan)
Ah yes!

So we can add a custom bug field for this. There is one constraint and one small bug:

- the relationship is asymmetric, a bug can only have one 'is regressed by' bug.
- a code change is required because in the default view, the new custom field won't show up.
Flags: needinfo?(dylan)
(In reply to Dylan Hardison [:dylan] (he/him) from comment #17)
> - the relationship is asymmetric, a bug can only have one 'is regressed by'
> bug.

I think this is OK, at least for now, given comment 11.
Great news Dylan, thanks!
Please prioritize the code change. 

There was a recent meeting to review the definition of the regression keyword used in Bugzilla. 

Is this something that :kohei could do?
Flags: needinfo?(kohei.yoshino)
Flags: needinfo?(dylan)
Sure, it’s something like Mentors and Crash Signature fields, I guess.
Flags: needinfo?(kohei.yoshino)
But, is that has to be a custom field? Regression tracking is not specific to BMO. It should be useful for other Bugzilla instances as well.
(In reply to Kohei Yoshino [:kohei] from comment #22)
> But, is that has to be a custom field? Regression tracking is not specific
> to BMO. It should be useful for other Bugzilla instances as well.

What do you think, :dylan?
(In reply to Kohei Yoshino [:kohei] from comment #23)
> (In reply to Kohei Yoshino [:kohei] from comment #22)
> > But, is that has to be a custom field? Regression tracking is not specific
> > to BMO. It should be useful for other Bugzilla instances as well.
> 
> What do you think, :dylan?

This would be much simpler for now as a custom field and if other installs want to have something like this they could also just use a custom field. THis way it is not enforced on all installs.

(In reply to Kohei Yoshino [:kohei] from comment #21)
> Sure, it’s something like Mentors and Crash Signature fields, I guess.

Yes. I think this is something trivial and you could go ahead and work on it. I can help out if needed.

dkl
Blocks: 1152434
Putting in my queue this week.
Assignee: dylan → kohei.yoshino
Component: Administration → General
We need to document this field's behavior, how we expect it to be used, and what we plan to do with existing bugs. 

Can we tell from a bug with the regression keyword and a non-empty blocks field what the relationship is?
What do you need from me to drive this to being done?
Flags: needinfo?(dylan) → needinfo?(kohei.yoshino)
I think I just need a validation to complete the work. It’s basically a copy of the existing dependency stuff. Should be done soon.
Flags: needinfo?(kohei.yoshino)

I have restarted to work on this. PR should be ready in a few days 🤞

progress, progress... https://github.com/kyoshino/bmo/compare/bug-1461492-regressed-by

I still think these regression fields could be a built-in feature that can be enabled or disabled by admins, like aliases and see also, rather than having them as custom fields, given the user base of upstream Bugzilla. This kind of thing may not be suitable for an extension.

(In reply to Kohei Yoshino [:kohei] (Bugzilla UX) (FxSiteCompat) from comment #31)

progress, progress... https://github.com/kyoshino/bmo/compare/bug-1461492-regressed-by

I still think these regression fields could be a built-in feature that can be enabled or disabled by admins, like aliases and see also, rather than having them as custom fields, given the user base of upstream Bugzilla. This kind of thing may not be suitable for an extension.

That sounds fine to me. Would you want guidance on how to do that?

I think I’m almost done 😀 Will finish tomorrow.

Attached file GitHub Pull Request —

Here we go.

So my PR is finally ready, but I’d like to rethink about the field name that will be paired with regressed_by. I used regresses as Emma suggested earlier, but it may sound unnatural for some people. Field names should be nouns whenever possible, so how about regressions? Other possibilities include caused and introduced, both are verbs though.

Flags: needinfo?(sledru)
Flags: needinfo?(mcastelluccio)
Flags: needinfo?(ehumphries)

regressed_by is probably okay.

  • regressed_by ✅
  • caused_by
  • introduced_by

I’m talking about the opposite field:

  • regressions - noun; maybe the best?
  • regresses - Emma’s initial proposal
  • regressed - confusing ❌
  • causes - verb but may look like plural noun = regressed_by ❌
  • caused
  • introduces
  • introduced

(In reply to Kohei Yoshino [:kohei] (Bugzilla UX) (FxSiteCompat) from comment #37)

regressed_by is probably okay.

  • regressed_by ✅
  • caused_by
  • introduced_by

I’m talking about the opposite field:

  • regressions - noun; maybe the best?
  • regresses - Emma’s initial proposal
  • regressed - confusing ❌
  • causes - verb but may look like plural noun = regressed_by ❌
  • caused
  • introduces
  • introduced

According to the dictionary regresses is a plural noun so I feel like it would be ok.

Maybe caused_by and causes would be less confusing?

I wonder if we should not use something similar to regressed_by to make it clear that it is the opposite field

  • caused_regressions ?
Flags: needinfo?(sledru)

Sylvestre agreed to use regressions so I’ll make that change. Given that the SQL table name is also regressions, I’ll keep the table column name as regresses which no one will see. The UI label Regressions can be changed later if needed.

So regressed_by and regressions? It sounds fine to me.

For blocks and depends_on we used verbs though.

Flags: needinfo?(mcastelluccio)

(In reply to Marco Castelluccio [:marco] from comment #41)

For blocks and depends_on we used verbs though.

Yeah, those are very old fields, so ;) duplicates and most fields are nouns.

The regression keyword will be redundant once this is landed, so I think it can be disabled. If the regressed_by field is not empty, the bug is a regression.

(In reply to Kohei Yoshino [:kohei] (Bugzilla UX) (FxSiteCompat) from comment #43)

The regression keyword will be redundant once this is landed, so I think it can be disabled. If the regressed_by field is not empty, the bug is a regression.

What about bugs for which we have established they are a regression (by testing an earlier Firefox version where it does not reproduce), but we don't know which specific bug(s) regressed it?

(In reply to Botond Ballo [:botond] from comment #44)

(In reply to Kohei Yoshino [:kohei] (Bugzilla UX) (FxSiteCompat) from comment #43)

The regression keyword will be redundant once this is landed, so I think it can be disabled. If the regressed_by field is not empty, the bug is a regression.

What about bugs for which we have established they are a regression (by testing an earlier Firefox version where it does not reproduce), but we don't know which specific bug(s) regressed it?

Umm, forgot that case, which is valid for sure. But we actually have the Has Regression Range field as well... If the value is yes or no (not --- nor irrelevant), it's also a sign of a regression. We have too many underused and partially duplicated fields 😑

See Also: → 1527001

I'd keep all of them for now to reduce risk, and maybe phase Has Regression Range out over time.

+1 to this.

Keeping the keywords for now and using Autonag to identify bugs which have the regression keyword but not a regressed-by with help nudge people to fill in the field (and move bugs out of blocks/blocked-by.)

Depends on: 1527356
Blocks: 1527467
Depends on: 1527485
Depends on: 1529690
Blocks: 1534315
Depends on: 1537926

Merged to master.

Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
See Also: → 537749
Duplicate of this bug: 1344088
Blocks: 1344091
Type: defect → enhancement
Blocks: 1529764
No longer depends on: 1529764
You need to log in before you can comment on or make changes to this bug.