Bug suggestions text highlighting breaks when bug summaries contain quotes

RESOLVED FIXED

Status

Tree Management
Treeherder
P2
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: KWierso, Assigned: KWierso)

Tracking

Details

Attachments

(3 attachments)

(Assignee)

Description

2 years ago
Created attachment 8747799 [details]
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.

Updated

2 years ago
Summary: Treeherder doesn't match parts of log lines if they have quotes → Bug suggestions text highlighting breaks when bug summaries contain quotes
(Assignee)

Comment 2

2 years ago
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?
Priority: -- → P2
(Assignee)

Comment 4

2 years ago
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').
Flags: needinfo?(james)
Flags: needinfo?(cdawson)
(Assignee)

Comment 5

2 years ago
That diff also highlights the "<TOP_LEVEL>" from my previous comment.
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.
Flags: needinfo?(cdawson)
Created attachment 8770679 [details] [review]
[treeherder] KWierso:regex > mozilla:master
(Assignee)

Comment 8

2 years ago
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.
Attachment #8770679 - Flags: review?(james)
Flags: needinfo?(james)
Attachment #8770679 - Flags: review?(james)

Updated

2 years ago
Assignee: nobody → wkocher
Created attachment 8785753 [details] [review]
[treeherder] KWierso:1269416 > mozilla:master
(Assignee)

Comment 10

2 years ago
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.
Attachment #8785753 - Flags: review?(james)
Attachment #8785753 - Flags: review?(james) → review+

Comment 11

2 years ago
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.
(Assignee)

Updated

2 years ago
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED

Updated

2 years ago
Depends on: 1301906
You need to log in before you can comment on or make changes to this bug.