Created attachment 142148 [details] Patch

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

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

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

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

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

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

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?

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

(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

Created attachment 142364 [details] Patch

Created attachment 142367 [details] Patch

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

(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

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

Created attachment 142384 [details] Patch for landing

Comment on attachment 142384 [details] Patch for landing Clearing flags on attachment: 142384 Committed r117390: <http://trac.webkit.org/changeset/117390>

All reviewed patches have been landed. Closing bug.

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