Bug 118060 – Add a new find-resolved-bugs command to webkit-patch.

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Gábor Ábrahám]

I made an old bug tracker tool. It searches for bug id-s in the TestExpectations file and lists which has "RESOLVED" status.

Run example: ./oldbugvalidator.sh /home/path/to/TestExpectations

Now this is a WIP solution, but helped me a lot with closing old bugs.
Our plans are to rewrite it in python.

[Csaba Osztrogonác]

I think you missed to attach your WIP tool. :)

[Gábor Ábrahám]

Created attachment 205677 [details]
OldBugValidatorTool 

Somehow it wasn't attached to my comment.

[WebKit Commit Bot]

Attachment 205677 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files']" exit_code: 1
Total errors found: 0 in 0 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 205677 [details]
OldBugValidatorTool 

Attachment 205677 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/948649

New failing tests:
media/video-zoom.html

[Build Bot]

Created attachment 205698 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08  Port: mac-mountainlion  Platform: Mac OS X 10.8.3

[Gábor Ábrahám]

Created attachment 206245 [details]
OldBugValidatorTool Python

The implementation of Old Bug Validator in Python.

[WebKit Commit Bot]

Attachment 206245 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/Scripts/old-bug-validator.py']" exit_code: 1
Tools/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Peter Gal]

View in context: https://bugs.webkit.org/attachment.cgi?id=206245&action=review

> Tools/Scripts/old-bug-validator.py:34
> +parser = OptionParser(usage=usage, description="Checking the given TestExpectations file looking for \"https://bugs.webkit.org/show_bug.cgi?id=123\" or \"webkit.org/b/123\" styled bugs, and collect the ones with RESOLVED status. (RESOLVED FIXED, RESOLVED DUPLICATE, RESOLVED WONTFIX, ...).")

If you use " inside the string then don't escape it just use ' around the string instead of the " that way you don't need to escape the inner quotes.

> Tools/Scripts/old-bug-validator.py:67
> +    result = re.search("<span id='"'static_bug_status'"'>RESOLVED", html)

err... this string looks really weird. what do you wanted to do here?

[Peter Gal]

View in context: https://bugs.webkit.org/attachment.cgi?id=206245&action=review

> Tools/ChangeLog:5
> +        Reviewed by Csaba Osztrogonác.

I don't think this is reviewed yet. You should not fill this out before review :)

> Tools/Scripts/old-bug-validator.py:62
> +#get the bugzilla sites and look for the RESOLVED... status

just a nit: space after the # and start with a capital letter to form a sentence.

[Ryosuke Niwa]

Comment on attachment 206245 [details]
OldBugValidatorTool Python

View in context: https://bugs.webkit.org/attachment.cgi?id=206245&action=review

Thanks for posting the patch. However, this shouldn't be a standalone python script.
There is a code to parse TestExpectations under Tools/Scripts/webkitpy/layout_tests.
We should be using that parser instead. There is also a code to fetch Bugzilla data. See code in Tools/Scripts/webkitpy/tool.

> Tools/ChangeLog:3
> +        Implementing Old Bug Validator Tool in Python. The aim is to check TestExpectations file for RESOLVED bugs in an easier way.

This line should be of the bug summary & url. See other entries.
This long description should appear below Reviewed by line. Agan, see other entries.

Please read http://www.webkit.org/coding/contributing.html.

> Tools/Scripts/old-bug-validator.py:4
> +# Copyright (C) 2013 University of Szeged
> +# Copyright (C) 2013 Gabor Abraham <[e-mail]>
> +# All rights reserved.

Wrong copyright format. See other files.

[Csaba Osztrogonác]

(In reply to comment #10)
> Thanks for posting the patch. However, this shouldn't be a standalone python script.
Of course it can be easy to add it as a new webkit-patch command.

> There is a code to parse TestExpectations under Tools/Scripts/webkitpy/layout_tests.
> We should be using that parser instead. 

The TestExpectations parser in layout_tests/models/test_expectations.py
would be overkiller and it isn't suitable for this use case now.

It parses only "The new format for a test expectation line":
        [[bugs] [ "[" <configuration modifiers> "]" <name> [ "[" <expectations> "]" ["#" <comment>]

where bugs must be in "webkit.org/b/XXXXX" format. But there are 
URLs in the comment sections too in different formats. I don't think
if we should refactor test_expectations.py instead of the oneliner regexp.

> There is also a code to fetch Bugzilla data. See code in Tools/Scripts/webkitpy/tool.

It can be used if it doesn't have too much dependencies and 
if we can avoid the mandatory bugzilla authentication somehow.
Because it is a non-sense if we wan't to force the user to authenticate
if fetching a bugzilla URL doesn't need authentication at all.

Otherwise I still think that a urllib2.urlopen(...) oneliner is much more
simpler than solving all of these problems, because we want to integrate
everything to the existing webkitpy infrastructure regardless of expense.

[Ryosuke Niwa]

(In reply to comment #11)
> (In reply to comment #10)
> > There is a code to parse TestExpectations under Tools/Scripts/webkitpy/layout_tests.
> > We should be using that parser instead. 
> 
> The TestExpectations parser in layout_tests/models/test_expectations.py
> would be overkiller and it isn't suitable for this use case now.
> 
> It parses only "The new format for a test expectation line":
>         [[bugs] [ "[" <configuration modifiers> "]" <name> [ "[" <expectations> "]" ["#" <comment>]
> 
> where bugs must be in "webkit.org/b/XXXXX" format. But there are 
> URLs in the comment sections too in different formats. I don't think
> if we should refactor test_expectations.py instead of the oneliner regexp.

If there is a use case for parsing comments, then we should support that in test_expectations.py maybe as an option.

The problem with adding one line regular expression like this is that it'll make the future format change harder.

> > There is also a code to fetch Bugzilla data. See code in Tools/Scripts/webkitpy/tool.
> 
> It can be used if it doesn't have too much dependencies and 
> if we can avoid the mandatory bugzilla authentication somehow.
> Because it is a non-sense if we wan't to force the user to authenticate
> if fetching a bugzilla URL doesn't need authentication at all.

It's sensible to require authentication if bugs have restricted accesses.  We can certainly add a mode to avoid authentication to bugzilla code.

[Gábor Ábrahám]

Created attachment 206876 [details]
Proposed patch

Now it can be used as a webkit-patch command, and using Bugzilla class to check the bugs.

[Csaba Osztrogonác]

Comment on attachment 206876 [details]
Proposed patch

Only one thing is missing, the TestExpectations parsing by
layout_tests/models/test_expectations.py . But it would be
overkiller for this tool. I think we can live with it now,
and we can fix it later in a separated bug. So r=me.

[Csaba Osztrogonác]

Comment on attachment 206876 [details]
Proposed patch

Clearing flags on attachment: 206876

Committed r152779: <http://trac.webkit.org/changeset/152779>

[Csaba Osztrogonác]

All reviewed patches have been landed.  Closing bug.