1057377 - Bug suggestions: get_error_search_term() returns a search term even if it's in the blacklist

Status: RESOLVED FIXED

(Tree Management :: Treeherder, defect, P1)

Description

[Ed Morley [:emorley]]

Broken out from bug 1057359.

(In reply to Mauro Doglio [:mdoglio] from comment #5)
> I think the bug is here
> https://github.com/mozilla/treeherder-service/blob/master/treeherder/
> log_parser/utils.py#L77
> We should get rid of the 'or error_line' I guess. If the error line is not
> an helpful search term it should return None

Also we should probably change:
  if not error_line:
      return ""

To:
  if not error_line:
      return None

I'm happy to take this Mauro :-)

Comment 1

[Ed Morley [:emorley]]

16:23 <edmorley|sheriffduty> mdoglio: I've run tests against the patch to fix the blacklist, and I'm getting these failures https://pastebin.mozilla.org/6097199
16:24 <edmorley|sheriffduty> mdoglio: from what I can tell, the test is only checking that the number of bug suggestions matches exactly the number of error lines stored in all_errors?
16:24 <mdoglio> yes
16:24 <edmorley|sheriffduty> mdoglio: I don't think that makes sense, since there are not always suggestions for each error line
16:25 <mdoglio> let me check the code again, it sounds wrong

The sample log:
https://github.com/mozilla/treeherder-service/blob/52a653911e450fb5c5e9359438fd90a87c85b82a/tests/sample_data/logs/mozilla-inbound_ubuntu64_vm-debug_test-mochitest-other-bm53-tests1-linux-build122.logview.json#L12

The test:
https://github.com/mozilla/treeherder-service/blob/5b1eacfd774ccc5d38a65ca66f375cab8f0d44ef/tests/log_parser/test_tasks.py#L103

I believe the fixed blacklist means we're (correctly) no longer matching against lines like:
{
"line": "12:46:46 ERROR - Return code: 1",
"linenumber": 2283
}, 

I guess we could just remove them from the sample log, but really we need to have test coverage against both false positives and negatives, whereas at the moment we only catch false negatives.

Comment 2

[Ed Morley [:emorley]]

To clarify from our IRC conversation, I can't tell if it's either:

1) The output from mozilla-inbound_ubuntu64_vm-debug_test-mochitest-other-bm53-tests1-linux-build122.logview.json needs to match a sample artefact json dump somewhere else in the repo, that I haven't been able to find.

or

2) If the test isn't actually comparing the output from mozilla-inbound_ubuntu64_vm-debug_test-mochitest-other-bm53-tests1-linux-build122.logview.json against anything, and is instead making the assumption that there is always just one bug suggestion per error line (which isn't a correct assumption, so we'd need to adjust the tests).

Comment 3

[Ed Morley [:emorley]]

Attachment: WIP: Correct behaviour for search term blacklist & fall-back

Prior to this patch, we would return a search term even if it was on
the blacklist, due to the |return search_term or error_line|.

In addition, if we found a search term initially, but both that
search term and the whole error line were not suitable, we
didn't correctly return None.

Will need test fixes.

Comment 4

[Ed Morley [:emorley]]

Attachment: Correct behaviour for search term blacklist & fall-back

Prior to this patch, we would return a search term even if it was on the blacklist, due to the |return search_term or error_line|.

In addition, if we found a search term initially, but both that search term and the whole error line were not suitable, we didn't correctly return None.

This also fixes tasks.py so that if there were no search terms, we still return the bugs suggestions artefact for that error line, which is what the UI expects. However I'm not sure if bugs=None is what you meant and if you UI will cope with that (since it may be expected the 'open_recent' and 'all_others' keys in the dict).

Tests pass locally with this patch.

Comment 5

[Mauro Doglio [:mdoglio]]

:edmorley can you please create a PR for this? I'd like to see the green light from travis before merging this :-)

Comment 6

[Ed Morley [:emorley]]

Attachment: treeherder-service/pull/191

I've split the commit up into 4 pieces for clarity and opened a PR, which is looking green.

Comment 7

[Ed Morley [:emorley]]

https://github.com/mozilla/treeherder-service/commit/d9521688574981fdada0205412a3e8be59184141
https://github.com/mozilla/treeherder-service/commit/8d18034b84c3c029656bd97af0e4fb4d4b90e19c
https://github.com/mozilla/treeherder-service/commit/77a30c10b1a0f1b87b9d6978536fca5022795b81
https://github.com/mozilla/treeherder-service/commit/395096c9bd8fcb867f2e4df6307fdcb8dc90a09b