WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
48926
Using BUG/BUGWK in test_expectations is error prone, should use BUGCR/BUGWK
https://bugs.webkit.org/show_bug.cgi?id=48926
Summary
Using BUG/BUGWK in test_expectations is error prone, should use BUGCR/BUGWK
Benjamin (Ben) Kalman
Reported
2010-11-03 10:03:18 PDT
Using BUG/BUGWK in test_expectations is error prone, should use BUGCR/BUGWK
Attachments
Patch
(259.34 KB, patch)
2010-11-03 10:08 PDT
,
Benjamin (Ben) Kalman
no flags
Details
Formatted Diff
Diff
Patch
(222.67 KB, patch)
2010-12-08 14:35 PST
,
Benjamin (Ben) Kalman
no flags
Details
Formatted Diff
Diff
Script used to create the test_expectations.txt changes
(2.41 KB, text/plain)
2010-12-08 14:38 PST
,
Benjamin (Ben) Kalman
no flags
Details
Output from running rename.sh
(39.10 KB, text/plain)
2010-12-08 14:39 PST
,
Benjamin (Ben) Kalman
no flags
Details
Patch
(215.98 KB, patch)
2010-12-14 16:26 PST
,
Benjamin (Ben) Kalman
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Benjamin (Ben) Kalman
Comment 1
2010-11-03 10:08:56 PDT
Created
attachment 72833
[details]
Patch
Benjamin (Ben) Kalman
Comment 2
2010-11-03 10:10:33 PDT
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 3
2010-11-03 10:17:06 PDT
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
Comment 4
2010-12-08 14:35:30 PST
Created
attachment 75967
[details]
Patch
Benjamin (Ben) Kalman
Comment 5
2010-12-08 14:38:49 PST
Created
attachment 75969
[details]
Script used to create the test_expectations.txt changes
Benjamin (Ben) Kalman
Comment 6
2010-12-08 14:39:39 PST
Created
attachment 75970
[details]
Output from running rename.sh
Benjamin (Ben) Kalman
Comment 7
2010-12-08 14:44:06 PST
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 8
2010-12-09 09:48:35 PST
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)
Comment 9
2010-12-14 15:16:12 PST
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
Comment 10
2010-12-14 15:20:04 PST
(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)
Comment 11
2010-12-14 15:21:33 PST
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
Comment 12
2010-12-14 15:23:10 PST
(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
Comment 13
2010-12-14 16:26:32 PST
Created
attachment 76590
[details]
Patch
Ojan Vafai
Comment 14
2010-12-14 16:27:54 PST
Comment on
attachment 76590
[details]
Patch Will commit this shortly.
Ojan Vafai
Comment 15
2010-12-14 16:29:05 PST
Comment on
attachment 76590
[details]
Patch Clearing flags on attachment: 76590 Committed
r74070
: <
http://trac.webkit.org/changeset/74070
>
Ojan Vafai
Comment 16
2010-12-14 16:29:10 PST
All reviewed patches have been landed. Closing bug.
Eric Seidel (no email)
Comment 17
2010-12-14 17:11:13 PST
I believe this broke the python tests.
Benjamin (Ben) Kalman
Comment 18
2010-12-14 17:15:25 PST
Yep(In reply to
comment #17
)
> I believe this broke the python tests.
Yep.
WebKit Review Bot
Comment 19
2010-12-14 17:18:49 PST
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
Comment 20
2010-12-14 17:19:15 PST
(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
Comment 21
2010-12-14 17:20:01 PST
(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
Comment 22
2010-12-22 19:21:25 PST
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.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug