1020919 - TBPL tabs should have recognizable titles for single push Try pages

Status: RESOLVED FIXED

(Tree Management Graveyard :: TBPL, defect)

Description

[David Teller [:Yoric] - still alive but not very active]

I generally have 10-20 TBPL tabs opened. I need to quickly determine which tab maps to which experiment I'm running and the pushlog is not always sufficient.

Would it be possible to customize the try syntax and TBPL as follows?

  hg qref -m "Experimenting with the cosmic Gizmo try: ... "

produces a page in which the title is

 "Experimenting with the cosmic Gizmo [n]"

where [n] is the number of failed tests, as usual.

Comment 1

[Ed Morley [:emorley]]

[TBPL is going away once treeherder is ready, and treeherder changes more to a "single webapp page that has in-content tabs for multiple repos" approach (TBD whether this sticks or not though) - however any discussion here can feed into treeherder in-content tab titles too, so worth discussing anyway]

I like the idea, I'm just not sure whether everyone's workflow is similar enough to make this work in practice. Excuse me thinking out loud for bit...

Using a "take the commit message of the tip commit in the push (if non-empty), after the try syntax was stripped and the string trimmed of whitespace" approach, a single push try page with a tip commit of form:

https://tbpl.mozilla.org/?tree=Try&rev=2fdd34172ba6
>  Bug 1020835 - Addref IDWriteFontFile try: -b do -p win32,win64 -u all -t none 

Could give a tab title of:
[0] Try: Bug 1020835 - Addref IDWriteFontFile - TBPL

...which works great. (The number of failures are intentionally at the start of the tab title, by request, so they are visible when the tabs are narrow).

If the cleaned up commit message were empty, we just use the current generic Try tab title instead. However looking at try, this is a common case, since most people have an empty-diff top commit containing their try syntax, with the meat of the patches below.

We could instead supplement the above approach by iterating through the commits until we find a non-empty one after scrubbing of try syntax.

Some examples using this:

https://tbpl.mozilla.org/?tree=Try&rev=decd4441b2a0
> try: -b do -p emulator -u all -t none 
> [mq]: test.patch 

Could give:
[0] Try: [mq]: test.patch - TBPL
(and for bonus points we could remove the '[mq]' prefix)

https://tbpl.mozilla.org/?tree=Try&rev=7d349e62e247
> try: -b o -p emulator -u marionette-webapi -t none
> Bug 968709 - Add a marionette test case to Bluetooth set to make sure the URL of Marionette client is correct.
> Bug 968709 - Add a marionette test for pairing APIs of BluetoothAdapter. 
> Backed out changeset 52068b669c2a (bug 911209) for causing type errors in tests 

Could give:
[0] Try: Bug 968709 - Add a marionette test case to Bluetooth set to make sure the URL of Marionette client is correct - TBPL

...however this is a bit long - so I guess we'd need to cap it?

Unfortunately for these cases, it wouldn't work so well:

https://tbpl.mozilla.org/?tree=Try&rev=728bbe3c2d95
> try: -b d -f -p linux -u jittests,jittest-1,jittest-2 -t none 
> Bug 1020110: Handle zero test cases 
> Bug 1019821: Run slow jit tests when asked to 

...a title of "[0] Try: Bug 1020110: Handle zero test cases" omits that fact that the push also included a patch for bug 1019821.

https://tbpl.mozilla.org/?tree=Try&rev=d838fe8d47eb
> try: -b do -p all -u all -t none
> Backed out changeset c9a08c041519 (bug 1020396) for Gu bustage on a CLOSED TREE 
> Bug 1000640 - Mark C/A 1.0/0.5 non-premult test as random, since it passes on Win7 Ru. - r=mattwoodrow CLOSED TREE 
> ...

...now for the above - the meat of the push was actually in the trychooser line - with the second commit just being an unrelated qparent - however we'd end up with a title based on the qparent instead.

Overall, we would need to:
 * Strip try syntax, [mq] prefix & flags(review/approval etc)
 * Limit the length - possibly being smart about trimming to just "Bug [0-9]" if present, so we don't get the bug number and then two unhelpful-without-context words. 
 * Strip whitespace
 * If the tree is !try or more than one try push is being displayed, just use the standard generic title
 * Either just attempt to use the tip commit (in which case most people won't see the updated title, but perhaps people will start modifying their trysyntax commit message once they know of the feature) or else iterate through the commit messages - but this will lead to occasional inaccurate titles, but will work for more pushes (though people might start to modify their commit messages to avoid false positives like the example above).

Comment 2

[Ed Morley [:emorley]]

Another alternative:

Only iterate through the commit messages if the next commit message was by the same author as the current commit. This would hopefully avoid misleading titles from unrelated qparents - although this would mean people pushing patches on behalf of someone else won't get the tab titles, even though they would have been accurate for that case.

Comment 3

[David Teller [:Yoric] - still alive but not very active]

FWIW, right now, my process is to open the devtools and change the document.title to differentiate the tabs.

Comment 4

[Ed Morley [:emorley]]

Attachment: WIP

David - don't know if you are up for giving this a go and letting me know if it works as you intended?

Apply the patch to https://hg.mozilla.org/webtools/tbpl/ and just run index.html (no need to use the Vagrant VM, since UI only changes are testable from the local filesystem since the paths auto-adjust to use the production backend).

Will need some more comments & patches splitting up, but seems to work reasonably well as is.

Comment 5

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8435235 [details] [diff] [review]
WIP

Review of attachment 8435235 [details] [diff] [review]:
-----------------------------------------------------------------

This certainly looks nicer.
In my latest use case, however, I am applying the same patch at different places in the tree, for bissection purposes, so this would always return the same message, wouldn't it?

Comment 6

[Ed Morley [:emorley]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #5)
> This certainly looks nicer.
> In my latest use case, however, I am applying the same patch at different
> places in the tree, for bissection purposes, so this would always return the
> same message, wouldn't it?

Only if you keep the top-most useable commit message the same each time - which isn't what you asked for in comment 0?

The current patch will work for this new use case as long as you do either:

> try: <normal syntax>
> Bug 123456 - Base commit foobar

-> tab title will include: "Bug 123456 - Base commit foobar" in the tab title (since we ignored the 'try:' line, since nothing useable there)

or if you use:

> testing against base commit foobar; try: <normal syntax>
> Bug 123456 - Base commit foobar

-> tab title will include: "testing against base commit A"

Comment 7

[Ed Morley [:emorley]]

(To be clear, the two '>' quoted lines were example separate commits, not a multi-line commit message)

Comment 8

[David Teller [:Yoric] - still alive but not very active]

Sorry, I head looked too quickly.
Yes, this is better, thanks. Do we really need to have "Try" in the topic, though? Since I have many tabs, this basically takes all the available space.

Comment 9

[Ed Morley [:emorley]]

Attachment: Part 1: Make the handleUpdatedPushes() param name clearer

'push' read to me as a single push, rather than what it is: an array of the updated pushes.

Also my editor miss highlights 'push' when used as a variable name, given Array push()

Comment 10

[Ed Morley [:emorley]]

Attachment: Part 2: Pass pushes array to initialPushlogLoadCallback

We're going to need the list of pushes shortly.

'unsortedPushes' since unlike the 'updatedPushes' above, it hasn't been sorted (didn't seem worth the overhead, since for my use in later patches, it doesn't need to be).

Comment 11

[Ed Morley [:emorley]]

Attachment: Part 3: Use a stored page title

...and update it on pageload & each time the pushlog is loaded.

We'll be soon having a page title that depends on the contents of the loaded
pushlog. As such, as part of handleInitialPushlogLoad() we calculate the new
title and store it for later use by _updateFailingBuildsDisplay(), which will be
called multiple times from the handleUpdatedPushes() callback firing when job
results have finished loading.

As an added bonus, it means we can removed the slightly magic-y way of updating the title in _updateFailingBuildsDisplay() using replace().

Comment 12

[Ed Morley [:emorley]]

Attachment: Part 4: Give single push Try tabs custom titles

For single-push only try pages, we iterate through the commits looking for a usable description, which we can then add to the title.

To both give us the cleanest page titles possible, and to cater for cases where the top commit is just something like:
"try: -b do -p foo - u bar"
...we strip off try syntax, as well as a bunch of other cruft - and if there is nothing left of the commit message, we just move onto the next commit.

Seems to work well for 90% of Try pushes - the rest just fall back to the standard title ("Try - TBPL") if no usable description was found.

The only case where we would get false positives, are if someone pushes just one commit to Try, which only has a trychooser line with no other text prior to it - and their push was the first to introduce a mozilla-inbound (or whatever repo) commit to Try. The tab title would reference the qparent commit, rather than being the fallback "Try - TBPL" title. However these cases are the exception rather than the norm (from looking at Try), and I think it's both pretty obvious what is happening when people see it, and will occur less and less, once people know they can name their try tabs by commit message (I'll post to the newsgroups once this lands).

Comment 13

[Ed Morley [:emorley]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #8)
> Yes, this is better, thanks. Do we really need to have "Try" in the topic,
> though? Since I have many tabs, this basically takes all the available space.

Meant to say I switched the order to stop the tree name reducing the amount of useful description visible - the patches as attached do:
[0] Foo bar baz - Try - TBPL

Comment 14

[Arpad Borsos [:Swatinem]]

Comment on attachment 8443502 [details] [diff] [review]
Part 3: Use a stored page title

Review of attachment 8443502 [details] [diff] [review]:
-----------------------------------------------------------------

Hm, ideally there should be just one method `updateTitle` or something like that, instead of 2½ places that modify document.title directly. Lets see what the next patch looks like.

Comment 15

[Ed Morley [:emorley]]

Attachment: Part 3: Use a stored page title

Added a _setTitlePrefix() method to avoid the two calls, since whilst part 4 made them make a bit more sense, it could still be done a bit more cleanly.

Comment 16

[Ed Morley [:emorley]]

Attachment: Part 4: Give single push Try tabs custom titles

Updated to fit with the new part 3.

Comment 17

[Ed Morley [:emorley]]

remote:   https://hg.mozilla.org/webtools/tbpl/rev/1a04965c7843
remote:   https://hg.mozilla.org/webtools/tbpl/rev/8c668853af63
remote:   https://hg.mozilla.org/webtools/tbpl/rev/f7cf9d718c6c
remote:   https://hg.mozilla.org/webtools/tbpl/rev/470fce047d3b