1269416 - Bug suggestions text highlighting breaks when bug summaries contain quotes

Status: RESOLVED FIXED

(Tree Management :: Treeherder, defect, P2)

Description

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Attachment: treeherderquotes.png

If a part of a failure line includes a double or single quote, the word that follows the quote doesn't get matched correctly.

Comment 1

[Ed Morley [:emorley]]

The code that does this highlighting is:
https://github.com/mozilla/treeherder/blob/fa61debc42ea36483048e04aa35250841abfc0bf/ui/js/filters.js#L89-L111

Comment 2

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Could've sworn this used to work. Maybe this is fallout from bug 1252854 or bug 1177519, both of which landed in the last few weeks?

Comment 3

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Looks like we also don't match this: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=b77b09e1e42037a34e9fbcfe1a39b3ec7f59e11d&selectedJob=27151258&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified

That "<TOP_LEVEL>" should be highlighted also.

Comment 4

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Cam, do you remember the reasoning for why we don't <strong>ify things inside of tags at https://github.com/mozilla/treeherder/blob/master/ui/js/filters.js#L146 ?

You changed the logic in this filter in https://github.com/mozilla/treeherder/commit/78b4378b43855cf01e53bfcf63013ec775a6ec70#diff-f3c6d859d44fcbc59c5c40ce6e7cb037 but the reasoning for not highlighting inside tags is older than that.

I'm quite sure highlighting inside quotes worked prior to your patch. 

I can get it working again in the failure summary panel if I do something like this, but I don't know what I'd be breaking by doing it:

diff --git a/ui/js/filters.js b/ui/js/filters.js
index 53abf16..a3aeedd 100755
--- a/ui/js/filters.js
+++ b/ui/js/filters.js
@@ -143,8 +143,10 @@ treeherder.filter('highlightCommonTerms', function() {
         angular.forEach(tokens, function(elem) {
             if (elem.length > 0) {
                 input = input.replace(new RegExp("(^|\\W)(" + elem + ")($|\\W)", "gi"), function(match, prefix, token, suffix, index, str) {
-                    if (inTag(str, index, "<", ">") || inTag(str, index, "&", ";")){
+                    if (inTag(str, index, "<", ">")) {
                         return match;
+                    } else if (inTag(str, index, "&", ";")) {
+                        return prefix + "<strong>" + token + "</strong>" + suffix;
                     } else {
                         return prefix + "<strong>" + token + "</strong>" + suffix;
                     }



I guess maybe this could mess up the autoclassify panel somehow?



I'm looking at http://localhost:8000/#/jobs?repo=mozilla-inbound&bugfiler&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified&tochange=f7a95ad1275001fa5ef8028fa557d89f6528ad43&fromchange=f7a95ad1275001fa5ef8028fa557d89f6528ad43&selectedJob=31581323&autoclassify (running with web-server.js) and without my diff applied, the failure summary panel fails to highlight most of the bug suggestion title after 'logcat', and the same is true for the autoclassify panel.

With that diff applied, the failure summary panel highlights pretty much the whole bug title, while the autoclassify panel doesn't highlight everything, but it does highlight more of it (up to 'dalvikvm:I').

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

That diff also highlights the "<TOP_LEVEL>" from my previous comment.

Comment 6

[Cameron Dawson [:camd]]

Mentioned this in IRC, but I literally have NoMemory(tm) of making that change.  :)  I'm pretty sure this was code that got merged together with stuff James was doing and my name ended up on it.  But I can't think of a reason not to try whatever change seems right to you and just test it.

James may have more thoughts on it.  But dissecting unfamiliar regex is on my list of un-favorite things just after shaving my head with a cheese-grater while chewing on tinfoil.

Comment 7

[GitHub Autolander Bot]

Attachment: [treeherder] KWierso:regex > mozilla:master

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Comment on attachment 8770679 [details] [review]
[treeherder] KWierso:regex > mozilla:master

No clue who should review this or if this should even be done. I wonder if I should only do the <strong>ifying when the "tag" we're in is a single or double quote?

I'm really not sure why we aren't <strong>ifying tags in the first place.

Comment 9

[GitHub Autolander Bot]

Attachment: [treeherder] KWierso:1269416 > mozilla:master

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Comment on attachment 8785753 [details] [review]
[treeherder] KWierso:1269416 > mozilla:master

How about this instead of the other PR? 

Since no one can remember why we were using inTag, I've deleted the inTag function and am now highlighting anything that matches between the bug summary and the failure line. 

Additionally, this makes the autoclassify panel match another section of the failure line against bug summaries that wasn't being compared before. This appears to highlight more of the line than it did before, but I don't know if there was a reason we weren't comparing against that section.

Comment 11

[Treeherder GitHub Bugbot]

Commit pushed to master at https://github.com/mozilla/treeherder

https://github.com/mozilla/treeherder/commit/c7ea6c9979049e740cae2df1ef9d5e1dc910e5fe
Bug 1269416 - Improve bug summary line highlighting (#1818)

This stops checking if we're in a tag, instead highlighting everything
that matches the failure line.

It also fixes the line highlighting in the autoclassify panel by checking
against more of the failure line than was previously included.