Bug 48926 – Using BUG/BUGWK in test_expectations is error prone, should use BUGCR/BUGWK

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Benjamin (Ben) Kalman]

Using BUG/BUGWK in test_expectations is error prone, should use BUGCR/BUGWK

[Benjamin (Ben) Kalman]

Created attachment 72833 [details]
Patch

[Benjamin (Ben) Kalman]

FYI, script to update the expectations files was

for f in `find LayoutTests -name '*_expectations.txt'`; do
  sed -E -i '' "s/BUG([0-9]+)/BUGCR\1/g" "$f"
done

[Ojan Vafai]

Comment on attachment 72833 [details]
Patch

Please send out and email to chromium-dev announcing this is about to change. Then feel free to commit. You'll need to make sure you're synced as this file changes frequently.

[Benjamin (Ben) Kalman]

Created attachment 75967 [details]
Patch

[Benjamin (Ben) Kalman]

Created attachment 75969 [details]
Script used to create the test_expectations.txt changes

[Benjamin (Ben) Kalman]

Created attachment 75970 [details]
Output from running rename.sh

[Benjamin (Ben) Kalman]

Ok, finally revisited this (i.e. synced + re-ran script -- which takes forever).

Just an update, I noticed that there were more than a few uses of BUG to indicate a webkit bug (BUGWK) rather than a chromium bug (BUGCR).  Rather than sorting through hundreds of them myself, rename.sh attempts to determine (via wget + grep) which it should be.

Sometimes the script fails.  Last time I ran it I actually did resolve these by hands, but didn't this time.  I attached the output of running the script (rename.out) just for the record.

[Ojan Vafai]

Comment on attachment 75967 [details]
Patch

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

This looks great!

> LayoutTests/ChangeLog:9
> +	For all test_expectations.txt files, change all occurrences of BUG to
> +	either BUGCR or BUGWK.

Indentation is off here. Use spaces, not tabs.

> WebKitTools/ChangeLog:9
> +	Add presubmit check that BUG isn't used, either BUGCR or BUGWK.

ditto re: spaces

> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_expectations.py:600
> +                self._add_error(lineno, 'Bug must be either BUGCR or BUGWK for test: %s' %

Nit: now there is also BUGV8_XXX

[Eric Seidel (no email)]

Ojan:  Benjamin is not a committer.  Should this be marked cq+?  I expect he's going to need to upload a new patch in which case we should probably mark it r-.

[Benjamin (Ben) Kalman]

(In reply to comment #9)
> Ojan:  Benjamin is not a committer.  Should this be marked cq+?  I expect he's going to need to upload a new patch in which case we should probably mark it r-.

Every time I upload this patch it already seems to be obsolete i.e. needs a non-trivial merge.  It touches ~half the lines in each expectations file so that's not surprising.

I was thinking about submitting it 500 lines at a time, or at a time when everybody is asleep (i.e. in my (Sydney) time zone) -- though unfortunately that also means that most committers are also asleep.  Maybe I can contact somebody in Tokyo.

[Eric Seidel (no email)]

There are definitely some changes which get much easier once you're a committer. :)  Seems you need about 4 more patches before you can be nominated though.

[Benjamin (Ben) Kalman]

(In reply to comment #11)
> There are definitely some changes which get much easier once you're a committer. :)  Seems you need about 4 more patches before you can be nominated though.

Yeah this change can presumably wait until then.

(However, if this is submitted then I need one less... hmm.)

[Benjamin (Ben) Kalman]

Created attachment 76590 [details]
Patch

[Ojan Vafai]

Comment on attachment 76590 [details]
Patch

Will commit this shortly.

[Ojan Vafai]

Comment on attachment 76590 [details]
Patch

Clearing flags on attachment: 76590

Committed r74070: <http://trac.webkit.org/changeset/74070>

[Ojan Vafai]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

I believe this broke the python tests.

[Benjamin (Ben) Kalman]

Yep(In reply to comment #17)
> I believe this broke the python tests.

Yep.

[WebKit Review Bot]

http://trac.webkit.org/changeset/74070 might have broken SnowLeopard Intel Release (Tests)
The following tests are not passing:
fast/css/focus-ring-detached.html
fast/css/focus-ring-multiline.html

[Benjamin (Ben) Kalman]

(In reply to comment #18)
> Yep(In reply to comment #17)
> > I believe this broke the python tests.
> 
> Yep.

Just need to change the occurrences of BUG1234 with BUGCR1234 in WebKitTools/Scripts/webkitpy/style/checkers/test_expectations_unittest.py

[Benjamin (Ben) Kalman]

(In reply to comment #19)
> http://trac.webkit.org/changeset/74070 might have broken SnowLeopard Intel Release (Tests)
> The following tests are not passing:
> fast/css/focus-ring-detached.html
> fast/css/focus-ring-multiline.html

I don't think so.

[Dirk Pranke]

For what it's worth, this patch should've had a test added to webkitpy/layout_tests/layout_package/test_expectations.py .

And, there's no need to be duplicating all of this unit testing logic in two different files. The style checker file should just be a trivial wrapper that creates a TestExpectations object and logs any errors that occur. 

I stumbled across this duplication (yet again) while testing the changes for bug 51222. I wouldn't be surprised if some of the style checking testing predated some of the unit tests, and prior to the cleanup in 51222, figuring out if things linted correctly was a little harder than it should be. I'll submit a separate patch to de-dup the code.