Bug 76278

Summary: [NRWT] Support --ignore-metrics
Product: WebKit Reporter: Balazs Kelemen <kbalazs>
Component: Tools / TestsAssignee: Balazs Kelemen <kbalazs>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dpranke, eric, hugo.lima, ojan, ossy, vestbo, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
none
fixed style
none
Patch
none
Patch none

Description Balazs Kelemen 2012-01-13 08:27:14 PST
The --ignore-metrics option of ORWT can be useful for getting basic coverage on platforms without maintaining baselines for the specific rendering backend. Maintaining metric baselines needs a lot of manpower and the metric mismatches are usually do not indicate real bugs. For the Qt port, it could be useful to have better coverage on non-Linux platforms (Mac, Windows, etc.).
Comment 1 Balazs Kelemen 2012-01-13 08:31:02 PST
Created attachment 122432 [details]
patch
Comment 2 WebKit Review Bot 2012-01-13 08:32:53 PST
Attachment 122432 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/Scripts/webkitpy..." exit_code: 1

Tools/Scripts/webkitpy/layout_tests/port/driver_unittest.py:37:  expected 2 blank lines, found 1  [pep8/E302] [5]
Tools/Scripts/webkitpy/layout_tests/port/driver_unittest.py:69:  whitespace before ']'  [pep8/E202] [5]
Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:313:  whitespace before ']'  [pep8/E202] [5]
Total errors found: 3 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Balazs Kelemen 2012-01-13 08:54:29 PST
Created attachment 122434 [details]
fixed style
Comment 4 Tony Chang 2012-01-13 10:38:02 PST
Comment on attachment 122434 [details]
fixed style

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

Some minor python style nits.  Overall, the patch seems OK.

> Tools/Scripts/webkitpy/layout_tests/port/driver.py:67
> +        strip_patterns = self.__class__.strip_patterns
> +        if strip_patterns == []:

You can just define the patterns above and avoid the if check.  Also, self.strip_patterns will automatically fall back to self.__class__.strip_patterns.

> Tools/Scripts/webkitpy/layout_tests/port/driver.py:80
> +            strip_patterns.append((re.compile('\n( *)"\s+'), r'\n\1"'))

r'\n\1' becomes \n\1 (no newline, actually backslash-n-backslash-1).  I think you meant to do '\n\\1' instead.  I'm not sure if a test case covers this regex.

> Tools/Scripts/webkitpy/layout_tests/port/driver_unittest.py:57
> +            ('text run at (0,0) width 109: ".one {color: green;}"\n\
> +          text run at (109,0) width 0: " "\n\
> +          text run at (0,17) width 81: ".1 {color: red;}"\n\
> +          text run at (81,17) width 0: " "\n\

You can use triple quotes and avoid the need to manually add \n\.
Comment 5 Balazs Kelemen 2012-01-16 09:48:57 PST
Created attachment 122655 [details]
Patch
Comment 6 Balazs Kelemen 2012-01-16 09:55:00 PST
(In reply to comment #4)
> (From update of attachment 122434 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=122434&action=review
> 
> Some minor python style nits.  Overall, the patch seems OK.
> 
> > Tools/Scripts/webkitpy/layout_tests/port/driver.py:67
> > +        strip_patterns = self.__class__.strip_patterns
> > +        if strip_patterns == []:
> 
> You can just define the patterns above and avoid the if check.  Also, self.strip_patterns will automatically fall back to self.__class__.strip_patterns.

Done.

> 
> > Tools/Scripts/webkitpy/layout_tests/port/driver.py:80
> > +            strip_patterns.append((re.compile('\n( *)"\s+'), r'\n\1"'))
> 
> r'\n\1' becomes \n\1 (no newline, actually backslash-n-backslash-1).  I think you meant to do '\n\\1' instead.  I'm not sure if a test case covers this regex.

I believe this rule should remove trailing spaces after text between quotes but keep the identation of the line. It seems like redundant since the '" +$' -> '"' rule should do the same so I removed this one.

> 
> > Tools/Scripts/webkitpy/layout_tests/port/driver_unittest.py:57
> > +            ('text run at (0,0) width 109: ".one {color: green;}"\n\
> > +          text run at (109,0) width 0: " "\n\
> > +          text run at (0,17) width 81: ".1 {color: red;}"\n\
> > +          text run at (81,17) width 0: " "\n\
> 
> You can use triple quotes and avoid the need to manually add \n\.

The identations should not change and I guess it would be against the style with triple quotes and it's easier to read it this way.
Comment 7 Balazs Kelemen 2012-01-17 02:11:49 PST
Created attachment 122737 [details]
Patch

Fixed the case of missing expected txt that caused an unhandled exception with the previous patch.
Comment 8 Balazs Kelemen 2012-01-26 02:10:53 PST
Committed r105981: <http://trac.webkit.org/changeset/105981>
Comment 9 Balazs Kelemen 2012-01-26 02:11:27 PST
(In reply to comment #8)
> Committed r105981: <http://trac.webkit.org/changeset/105981>

Landed with the triple quote fix.
Comment 10 Hugo Parente Lima 2012-01-26 13:28:43 PST
I think it's not fully working yet, running the tests with:

new-run-webkit-tests --ignore-metrics -2 --no-launch-safari --no-new-test-results --no-sample-on-timeout --results-directory /tmp/bug --release --exit-after-n-crashes-or-timeouts 20 --exit-after-n-failures 500 --qt

I got a lot of failures with diffs like:

                 RenderText {#text} 
-                  " was set."
+                  "was set."

Tested with svn rev 106030, (496b672dab88013d53891821c856aa73aa553130 on git)
Comment 11 Balazs Kelemen 2012-01-26 15:15:55 PST
(In reply to comment #10)
> I think it's not fully working yet, running the tests with:
> 
> new-run-webkit-tests --ignore-metrics -2 --no-launch-safari --no-new-test-results --no-sample-on-timeout --results-directory /tmp/bug --release --exit-after-n-crashes-or-timeouts 20 --exit-after-n-failures 500 --qt
> 
> I got a lot of failures with diffs like:
> 
>                  RenderText {#text} 
> -                  " was set."
> +                  "was set."
> 
> Tested with svn rev 106030, (496b672dab88013d53891821c856aa73aa553130 on git)

I will look into that tomorrow. Thanks for pointing it out.
Comment 12 Balazs Kelemen 2012-01-30 04:16:43 PST
(In reply to comment #11)
> (In reply to comment #10)
> > I think it's not fully working yet, running the tests with:
> > 
> > new-run-webkit-tests --ignore-metrics -2 --no-launch-safari --no-new-test-results --no-sample-on-timeout --results-directory /tmp/bug --release --exit-after-n-crashes-or-timeouts 20 --exit-after-n-failures 500 --qt
> > 
> > I got a lot of failures with diffs like:
> > 
> >                  RenderText {#text} 
> > -                  " was set."
> > +                  "was set."
> > 
> > Tested with svn rev 106030, (496b672dab88013d53891821c856aa73aa553130 on git)
> 
> I will look into that tomorrow. Thanks for pointing it out.

I could not reproduce this bug. Not all tests pass for me locally but I have no failures like yours (and I think my failures are not connected with --ignore-metrics).
Comment 13 Eric Seidel (no email) 2012-10-16 15:39:27 PDT
This feature seems questionable at best.  But I think it was a mistake to add this into driver.py, instead of creating a separate object for this post-processing hack. :(