Bug 76278 – [NRWT] Support --ignore-metrics

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

[Balazs Kelemen]

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.).

[Balazs Kelemen]

Created attachment 122432 [details]
patch

[WebKit Review Bot]

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.

[Balazs Kelemen]

Created attachment 122434 [details]
fixed style

[Tony Chang]

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\.

[Balazs Kelemen]

Created attachment 122655 [details]
Patch

[Balazs Kelemen]

(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.

[Balazs Kelemen]

Created attachment 122737 [details]
Patch

Fixed the case of missing expected txt that caused an unhandled exception with the previous patch.

[Balazs Kelemen]

Committed r105981: <http://trac.webkit.org/changeset/105981>

[Balazs Kelemen]

(In reply to comment #8)
> Committed r105981: <http://trac.webkit.org/changeset/105981>

Landed with the triple quote fix.

[Hugo Parente Lima]

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)

[Balazs Kelemen]

(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.

[Balazs Kelemen]

(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).

[Eric Seidel (no email)]

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. :(