Bug 86575 - Add tests for CSS Variables.
Summary: Add tests for CSS Variables.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Luke Macpherson
URL:
Keywords:
Depends on:
Blocks: 85580
  Show dependency treegraph
 
Reported: 2012-05-15 22:23 PDT by Luke Macpherson
Modified: 2012-06-29 15:05 PDT (History)
7 users (show)

See Also:


Attachments
Patch (17.55 KB, patch)
2012-05-15 22:24 PDT, Luke Macpherson
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-03 (699.24 KB, application/zip)
2012-05-16 02:31 PDT, WebKit Review Bot
no flags Details
Archive of layout-test-results from ec2-cq-01 (555.83 KB, application/zip)
2012-05-16 09:52 PDT, WebKit Review Bot
no flags Details
Patch (18.37 KB, patch)
2012-05-16 16:26 PDT, Luke Macpherson
no flags Details | Formatted Diff | Diff
Patch (21.62 KB, patch)
2012-05-16 16:34 PDT, Luke Macpherson
no flags Details | Formatted Diff | Diff
Patch for landing (21.70 KB, patch)
2012-05-16 18:21 PDT, Luke Macpherson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Luke Macpherson 2012-05-15 22:23:23 PDT
Add tests for CSS Variables.
Comment 1 Luke Macpherson 2012-05-15 22:24:16 PDT
Created attachment 142148 [details]
Patch
Comment 2 Luke Macpherson 2012-05-15 22:30:44 PDT
I'm a little confused as to the process for adding ref tests for a not-yet-implemented feature. Should I just change the -expected.html files to -expected-mismatch.html for now and then rename them back to -expected.html once the feature is implemented?

Right now these tests are disabled in Tools/Scripts/webkitpy/layout_tests/port/webkit.py when the CSS Variables data strucures are compiled out (Accidentally but harmlessly in https://bugs.webkit.org/show_bug.cgi?id=86338).
Comment 3 WebKit Review Bot 2012-05-16 02:31:23 PDT
Comment on attachment 142148 [details]
Patch

Attachment 142148 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12717152

New failing tests:
fast/css/variables/redefinition.html
fast/css/variables/shorthand.html
fast/css/variables/variable-chain.html
fast/css/variables/computed-style.html
fast/css/variables/use-before-defined.html
fast/css/variables/inline-styles.html
fast/css/variables/inherited-values.html
fast/css/variables/colors-test.html
fast/css/variables/var-inside-shorthand.html
Comment 4 WebKit Review Bot 2012-05-16 02:31:27 PDT
Created attachment 142204 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 5 Dimitri Glazkov (Google) 2012-05-16 09:19:11 PDT
(In reply to comment #2)
> I'm a little confused as to the process for adding ref tests for a not-yet-implemented feature. Should I just change the -expected.html files to -expected-mismatch.html for now and then rename them back to -expected.html once the feature is implemented?

I would just add them to test_expectations.txt and remove failed expectations as stuff lands.

Hmm.. Good question. Ryosuke, Dirk -- how would you go about this?

> 
> Right now these tests are disabled in Tools/Scripts/webkitpy/layout_tests/port/webkit.py when the CSS Variables data strucures are compiled out (Accidentally but harmlessly in https://bugs.webkit.org/show_bug.cgi?id=86338).
Comment 6 Ryosuke Niwa 2012-05-16 09:34:00 PDT
(In reply to comment #2)
> I'm a little confused as to the process for adding ref tests for a not-yet-implemented feature. Should I just change the -expected.html files to -expected-mismatch.html for now and then rename them back to -expected.html once the feature is implemented?

Add them to the skipped list.
Comment 7 WebKit Review Bot 2012-05-16 09:52:13 PDT
Comment on attachment 142148 [details]
Patch

Rejecting attachment 142148 [details] from commit-queue.

New failing tests:
fast/css/variables/redefinition.html
fast/css/variables/shorthand.html
fast/css/variables/variable-chain.html
fast/css/variables/computed-style.html
fast/css/variables/inline-styles.html
fast/css/variables/inherited-values.html
fast/css/variables/use-before-defined.html
fast/css/variables/colors-test.html
fast/css/variables/var-inside-shorthand.html
Full output: http://queues.webkit.org/results/12708595
Comment 8 WebKit Review Bot 2012-05-16 09:52:18 PDT
Created attachment 142284 [details]
Archive of layout-test-results from ec2-cq-01

The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: ec2-cq-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 9 Dirk Pranke 2012-05-16 12:17:23 PDT
(In reply to comment #6)
> (In reply to comment #2)
> > I'm a little confused as to the process for adding ref tests for a not-yet-implemented feature. Should I just change the -expected.html files to -expected-mismatch.html for now and then rename them back to -expected.html once the feature is implemented?
> 
> Add them to the skipped list.

That, or mark them as FAIL, and then you can actually override those expectations locally for own purposes using --additional-expectations (which is the path to another expectations file).
Comment 10 Luke Macpherson 2012-05-16 16:11:17 PDT
Should I only change the test_expectations.txt for chromium, as they are skipped by the script on other ports, or would it be normal to add them to every port's test_expectations.txt?
Comment 11 Ryosuke Niwa 2012-05-16 16:19:58 PDT
(In reply to comment #10)
> Should I only change the test_expectations.txt for chromium, as they are skipped by the script on other ports, or would it be normal to add them to every port's test_expectations.txt?

non-Chromium ports don't use test_expectations.txt. It seems okay not to add them if they're already skipped.
Comment 12 Dimitri Glazkov (Google) 2012-05-16 16:23:03 PDT
(In reply to comment #11)
> (In reply to comment #10)
> > Should I only change the test_expectations.txt for chromium, as they are skipped by the script on other ports, or would it be normal to add them to every port's test_expectations.txt?
> 
> non-Chromium ports don't use test_expectations.txt. It seems okay not to add them if they're already skipped.

That's not true anymore. Everybody uses test_expectations now: http://trac.webkit.org/browser/trunk/LayoutTests/platform/mac/test_expectations.txt
Comment 13 Luke Macpherson 2012-05-16 16:26:11 PDT
Created attachment 142364 [details]
Patch
Comment 14 Luke Macpherson 2012-05-16 16:34:44 PDT
Created attachment 142367 [details]
Patch
Comment 15 Ryosuke Niwa 2012-05-16 16:44:07 PDT
(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #10)
> > > Should I only change the test_expectations.txt for chromium, as they are skipped by the script on other ports, or would it be normal to add them to every port's test_expectations.txt?
> > 
> > non-Chromium ports don't use test_expectations.txt. It seems okay not to add them if they're already skipped.
> 
> That's not true anymore. Everybody uses test_expectations now: http://trac.webkit.org/browser/trunk/LayoutTests/platform/mac/test_expectations.txt

As far as I know, those are entries erroneously added by Chromium WebKit contributors or non-Apple contributors who don't understand the convention in Apple's ports.
Comment 16 Dimitri Glazkov (Google) 2012-05-16 16:45:56 PDT
(In reply to comment #15)
> (In reply to comment #12)
> > (In reply to comment #11)
> > > (In reply to comment #10)
> > > > Should I only change the test_expectations.txt for chromium, as they are skipped by the script on other ports, or would it be normal to add them to every port's test_expectations.txt?
> > > 
> > > non-Chromium ports don't use test_expectations.txt. It seems okay not to add them if they're already skipped.
> > 
> > That's not true anymore. Everybody uses test_expectations now: http://trac.webkit.org/browser/trunk/LayoutTests/platform/mac/test_expectations.txt
> 
> As far as I know, those are entries erroneously added by Chromium WebKit contributors or non-Apple contributors who don't understand the convention in Apple's ports.

Come on dude, look at the revision history :)

http://trac.webkit.org/log/trunk/LayoutTests/platform/mac/test_expectations.txt
Comment 17 Ryosuke Niwa 2012-05-16 16:47:14 PDT
(In reply to comment #16)
> Come on dude, look at the revision history :)
> 
> http://trac.webkit.org/log/trunk/LayoutTests/platform/mac/test_expectations.txt

Huh, so they have changed the convention then. Good to know.
Comment 18 Luke Macpherson 2012-05-16 18:21:41 PDT
Created attachment 142384 [details]
Patch for landing
Comment 19 WebKit Review Bot 2012-05-16 20:54:59 PDT
Comment on attachment 142384 [details]
Patch for landing

Clearing flags on attachment: 142384

Committed r117390: <http://trac.webkit.org/changeset/117390>
Comment 20 WebKit Review Bot 2012-05-16 20:55:05 PDT
All reviewed patches have been landed.  Closing bug.
Comment 21 Tony Chang 2012-06-29 15:05:55 PDT
I am late to the party, but most of these don't need to be ref tests.  These could just as easily be dumpAsText tests that use getComputedStyle to make sure the variables are resolved properly.  You could also use a variable with a width/height and query that the offsetWidth/offsetHeight is correct.

In general, dumpAsText tests can be easier to evaluate correctness (you don't nee do to think about how the -expected.html file renders) and they run faster (we don't have to grab the pixels and we don't have to diff images).