Support for @mixin, @include and @extend

pkg/csslib/test/mixin_test.dart

Index: pkg/csslib/test/mixin_test.dart
diff --git a/pkg/csslib/test/mixin_test.dart b/pkg/csslib/test/mixin_test.dart
new file mode 100644
index 0000000000000000000000000000000000000000..d1de2e33aa4006964e85c50c83ff3ec23aadaec9
--- /dev/null
+++ b/pkg/csslib/test/mixin_test.dart
@@ -0,0 +1,685 @@
+// Copyright (c) 2013, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+library mixin_test;
+
+import 'package:unittest/unittest.dart';
+import 'testing.dart';
+
+var  options = ['--warnings_as_errors', '--no-colors', 'memory'];

[Jennifer Messerly, 2013/09/18 06:44:15]

extra space here?

[nweiz, 2013/09/18 22:40:54]

"final", not "var".

[terry, 2013/10/09 03:40:33]

Done.

+
+void topLevelMixin() {

[nweiz, 2013/09/18 22:40:54]

Why are you defining a separate function for each test? pkg/unittest provides a
pre-existing way of dividing up and naming test cases; you should use that.

+  var errors = [];
+  final input1 = r'''

[nweiz, 2013/09/18 22:40:54]

Why is this [input1]? There's no other [input] variables in this scope. Same
goes for [generated1] and [stylesheet1].

This should also be "var", not "final", since it's a local variable.

[terry, 2013/10/09 03:40:33]

Probably other test at one point. 1 is gone.
On 2013/09/18 22:40:54, nweiz wrote:

+@mixin silly-links {
+  a {
+    color: blue;
+    background-color: red;
+  }
+}
+
+@include silly-links;
+''';

[nweiz, 2013/09/18 22:40:54]

These no-indentation strings are difficult to read, especially when [generated]
starts on the same line as the string opener. One thing Bob and I have done in
the past is do a small amount of pre-processing to allow the input and output to
be indented eight spaces in the source of the test.

[terry, 2013/10/09 03:40:33]

I'll consider it but I fine pre-processing to be a pain in test too.
On 2013/09/18 22:40:54, nweiz wrote:

+
+  final generated1 = r'''a {
+  color: #00f;
+  background-color: #f00;
+}''';
+
+  final stylesheet1 = compileCss(input1, errors: errors, opts: options);

[nweiz, 2013/09/18 22:40:54]

Using an out parameter like [errors] here is a code smell. Why isn't
[compileCss] just throwing an exception?

[terry, 2013/10/09 03:40:33]

It's something we've liked so far for tooling.
On 2013/09/18 22:40:54, nweiz wrote:

+
+  expect(stylesheet1 != null, true);

[nweiz, 2013/09/18 22:40:54]

This could be written as "expect(stylesheet, isNotNull)". However, I don't think
it makes sense to make this assertion at all. The feature you're testing --
@mixin -- has nothing to do with whether the stylesheet that's returned is null.
If it does happen to be null for some unexpected reason, other asserts will
catch that.

+  expect(errors.isEmpty, true, reason: errors.toString());

[nweiz, 2013/09/18 22:40:54]

expect(errors, isEmpty, ...)

+  expect(prettyPrint(stylesheet1), generated1);
+
+  // TODO(terry): Test using a declaration @mixin as a top-level @include.

[nweiz, 2013/09/18 22:40:54]

Why is this a TODO and not an actual test?

[terry, 2013/10/09 03:40:33]

comment old.
On 2013/09/18 22:40:54, nweiz wrote:

+}
+
+void topLevelMixin2Includes() {
+  final errors = [];
+  final input = r'''
+@mixin a {
+  a {
+    color: blue;
+    background-color: red;
+  }
+}
+@mixin b {
+  span {
+    color: black;
+    background-color: orange;
+  }
+}
+@include a;
+@include b;
+''';
+
+  final generated = r'''a {
+  color: #00f;
+  background-color: #f00;
+}
+span {
+  color: #000;
+  background-color: #ffa500;
+}''';
+
+  final stylesheet = compileCss(input, errors: errors, opts: options);
+
+  expect(stylesheet != null, true);
+  expect(errors.isEmpty, true, reason: errors.toString());
+  expect(prettyPrint(stylesheet), generated);
+}
+
+void topLevelMixinMultiRulesets() {

[nweiz, 2013/09/18 22:40:54]

What do multiple rulesets have to do with this test? It seems like the last test
tests multiple rulesets. This looks like it's testing transitive mixin
inclusion. If that's the case, it should get rid of "@mixin a", as that's not
contributing to the behavior being tested.

[terry, 2013/10/09 03:40:33]

Tests top-level mixins that includes another mixin
On 2013/09/18 22:40:54, nweiz wrote:

+  final errors = [];
+  final input = r'''
+@mixin a {
+  a {
+    color: blue;
+    background-color: red;
+  }
+}
+@mixin b {
+  span {
+    color: black;
+    background-color: orange;
+  }
+  @include c;
+}
+@mixin c {
+  #foo-id {
+    border-top: 1px solid red;
+    border-bottom: 2px solid green;
+  }
+}
+@include a;
+@include b;
+''';
+
+  final generated = r'''a {
+  color: #00f;
+  background-color: #f00;
+}
+span {
+  color: #000;
+  background-color: #ffa500;
+}
+#foo-id {
+  border-top: 1px solid #f00;
+  border-bottom: 2px solid #008000;
+}''';
+
+  final stylesheet = compileCss(input, errors: errors, opts: options);
+
+  expect(stylesheet != null, true);
+  expect(errors.isEmpty, true, reason: errors.toString());
+  expect(prettyPrint(stylesheet), generated);
+}
+
+void topLevelMixinMultiRulesets2() {

[nweiz, 2013/09/18 22:40:54]

I have no idea what this test is testing. Is it just... a lot of mixins? Why
would six mixins fail when two mixins didn't? This test seems superfluous.

[terry, 2013/10/09 03:40:33]

1, 2 and 3 nesting levels of includes.
On 2013/09/18 22:40:54, nweiz wrote:

+  final errors = [];
+  final input = r'''
+@mixin a {
+  a {
+    color: blue;
+    background-color: red;
+  }
+}
+@mixin b {
+  @include a;
+  span {
+    color: black;
+    background-color: orange;
+  }
+  @include c;
+  @include d;
+}
+@mixin c {
+  #foo-id {
+    border-top: 1px solid red;
+    border-bottom: 2px solid green;
+  }
+}
+@mixin d {
+  a:hover {
+    cursor: arrow;
+  }
+  @include e
+}
+@mixin e {
+  #split-bar:visited {
+    color: gray;
+  }
+  @include f;
+}
+@mixin f {
+  #split-bar div {
+    border: 1px solid lightgray;
+  }
+}
+@include b;
+''';
+
+  final generated = r'''a {
+  color: #00f;
+  background-color: #f00;
+}
+span {
+  color: #000;
+  background-color: #ffa500;
+}
+#foo-id {
+  border-top: 1px solid #f00;
+  border-bottom: 2px solid #008000;
+}
+a:hover {
+  cursor: arrow;
+}
+#split-bar:visited {
+  color: #808080;
+}
+#split-bar div {
+  border: 1px solid #d3d3d3;
+}''';
+
+  final stylesheet = compileCss(input, errors: errors, opts: options);
+
+  expect(stylesheet != null, true);
+  expect(errors.isEmpty, true, reason: errors.toString());
+  expect(prettyPrint(stylesheet), generated);
+}
+
+void topLevelMixinSelectors() {

[nweiz, 2013/09/18 22:40:54]

All the previous tests had selectors. Saying that this one tests selectors
doesn't tell me anything. Maybe this should be titled "test top-level mixin with
multiple selectors"?

[terry, 2013/10/09 03:40:33]

testing with combinators and selector groups.
On 2013/09/18 22:40:54, nweiz wrote:

+  final errors = [];
+  final input = r'''
+@mixin a {
+  a, b {
+    color: blue;
+    background-color: red;
+  }
+  div > span {
+    color: black;
+    background-color: orange;
+  }
+}
+
+@include a;
+''';
+
+  final generated = r'''a, b {
+  color: #00f;
+  background-color: #f00;
+}
+div > span {
+  color: #000;
+  background-color: #ffa500;
+}''';
+
+  final stylesheet = compileCss(input, errors: errors, opts: options);
+
+  expect(stylesheet != null, true);
+  expect(errors.isEmpty, true, reason: errors.toString());
+  expect(prettyPrint(stylesheet), generated);
+}
+
+void declSimpleMixin() {
+  final errors = [];
+  final input = r'''
+@mixin div-border {
+  border: 2px dashed red;
+}
+div {
+  @include div-border;
+}
+''';
+
+  final generated = r'''div {
+  border: 2px dashed #f00;
+}''';
+
+  var stylesheet = compileCss(input, errors: errors, opts: options);
+
+  expect(stylesheet != null, true);
+  expect(errors.isEmpty, true, reason: errors.toString());
+  expect(prettyPrint(stylesheet), generated);
+}
+
+void declMixin2Includes() {
+  final errors = [];
+  final input = r'''
+@mixin div-border {
+  border: 2px dashed red;
+}
+@mixin div-color {
+  color: blue;
+}
+div {
+  @include div-border;
+  @include div-color;
+}
+''';
+
+  final generated = r'''div {
+  border: 2px dashed #f00;
+  color: #00f;
+}''';
+
+  var stylesheet = compileCss(input, errors: errors, opts: options);
+
+  expect(stylesheet != null, true);
+  expect(errors.isEmpty, true, reason: errors.toString());
+  expect(prettyPrint(stylesheet), generated);
+}
+
+void declMixinNestedIncludes() {

[nweiz, 2013/09/18 22:40:54]

As above, "div-border" is unrelated to what this test is testing.

[terry, 2013/10/09 03:40:33]

As test implies for than one level of testing.
On 2013/09/18 22:40:54, nweiz wrote:

+  final errors = [];
+  final input = r'''
+@mixin div-border {
+  border: 2px dashed red;
+}
+@mixin div-color {
+  @include div-padding;
+  color: blue;
+  @include div-margin;
+}
+@mixin div-padding {
+  padding: .5em;
+}
+@mixin div-margin {
+  margin: 5px;
+}
+div {
+  @include div-border;
+  @include div-color;
+}
+''';
+
+  final generated = r'''div {
+  border: 2px dashed #f00;
+  padding: .5em;
+  color: #00f;
+  margin: 5px;
+}''';
+
+  var stylesheet = compileCss(input, errors: errors, opts: options);
+
+  expect(stylesheet != null, true);
+  expect(errors.isEmpty, true, reason: errors.toString());
+  expect(prettyPrint(stylesheet), generated);
+}
+
+void declMixinNestedIncludes2() {

[nweiz, 2013/09/18 22:40:54]

I can't understand what this test tests that the previous test didn't.

[terry, 2013/10/09 03:40:33]

nesting with other selectors pseudo.
On 2013/09/18 22:40:54, nweiz wrote:

+  final errors = [];
+  final input = r'''
+@mixin div-border {
+  border: 2px dashed red;
+}
+@mixin div-color {
+  @include div-padding;
+  @include div-margin;
+}
+@mixin div-padding {
+  padding: .5em;
+}
+@mixin div-margin {
+  margin: 5px;
+}
+div {
+  @include div-border;
+  @include div-color;
+}
+''';
+
+  final generated = r'''div {
+  border: 2px dashed #f00;
+  padding: .5em;
+  margin: 5px;
+}''';
+
+  var stylesheet = compileCss(input, errors: errors, opts: options);
+
+  expect(stylesheet != null, true);
+  expect(errors.isEmpty, true, reason: errors.toString());
+  expect(prettyPrint(stylesheet), generated);
+}
+
+void mixinArg() {
+  final errors = [];
+  final input = r'''
+@mixin div-border {
+  border: 2px dashed red;
+}
+
+@mixin div-inside(@dist) {

[nweiz, 2013/09/18 22:40:54]

If your arguments are going to use this syntax, you're not implementing SCSS,
you're writing your own idiosyncratic stylesheet language. That's fine if you
don't mind sacrificing user knowledge and cross-compatibility, but if you go
that route, you can't refer to them as "Sass mixins" or use ".scss" as the file
extension.

[terry, 2013/10/09 03:40:33]

Vars are now different in CSS (var-) but we're support @ as well.
On 2013/09/18 22:40:54, nweiz wrote:

+  margin-left: var(dist);
+  
+}
+
+div {
+  @include left(10px);
+  @include div-border;
+}
+''';
+
+  final generated = r''' ''';
+
+  var stylesheet = compileCss(input, errors: errors, opts: options);
+
+  expect(stylesheet != null, true);
+  expect(errors.isEmpty, true, reason: errors.toString());
+  expect(prettyPrint(stylesheet), generated);
+
+  // TODO(terry): Test using a top-level @mixin as a declaration @include.
+}
+
+void mixinArgs() {
+  final errors = [];
+  var input = r'''
+@mixin box-shadow(@shadows...) {
+  -moz-box-shadow: var(shadows);
+  -webkit-box-shadow: var(shadows;
+  box-shadow: var(shadows);
+}
+
+.shadows {
+  @include box-shadow(0px 4px 5px #666, 2px 6px 10px #999);
+}''';
+
+  var generated = r'''
+.shadowed {
+  -moz-box-shadow: 0px 4px 5px #666, 2px 6px 10px #999;
+  -webkit-box-shadow: 0px 4px 5px #666, 2px 6px 10px #999;
+  box-shadow: 0px 4px 5px #666, 2px 6px 10px #999;
+}
+''';
+
+  var stylesheet = compileCss(input, errors: errors, opts: options);
+
+  expect(stylesheet != null, true);
+  expect(errors.isEmpty, true, reason: errors.toString());
+  expect(prettyPrint(stylesheet), generated);
+}
+
+void mixinArgs_2() {
+  final errors = [];
+  var input = r'''
+@mixin colors(@text, @background, @border) {
+  color: var(text);
+  background-color: var(background);
+  border-color: var(border);
+}
+
+@values: #ff0000, #00ff00, #0000ff;
+.primary {
+  @include colors(var(values));
+}
+''';
+
+  var generated = r'''
+.primary {
+  color: #ff0000;
+  background-color: #00ff00;
+  border-color: #0000ff;
+}
+''';
+
+  var stylesheet = compileCss(input, errors: errors, opts: options);
+
+  expect(stylesheet != null, true);
+  expect(errors.isEmpty, true, reason: errors.toString());
+  expect(prettyPrint(stylesheet), generated);
+}
+
+void mixinArgs_var() {
+  final errors = [];
+  var input = r'''
+@mixin wrapped-stylish-mixin(@args...) {
+  font-weight: bold;
+  @include stylish-mixin(var(args));
+}
+
+.stylish {
+  // The @width argument will get passed on to "stylish-mixin" as a keyword
+  @include wrapped-stylish-mixin(#00ff00, @width: 100px);
+}
+''';
+
+  var generated = r'''
+''';
+
+  var stylesheet = compileCss(input, errors: errors, opts: options);
+
+  expect(stylesheet != null, true);
+  expect(errors.isEmpty, true, reason: errors.toString());
+  expect(prettyPrint(stylesheet), generated);
+}
+
+void mixin_content_toplevel() {
+  void mixinArgs_var() {
+    final errors = [];
+    var input = r'''
+@mixin apply-to-ie6-only {
+  * html {
+    @content;
+  }
+}
+@include apply-to-ie6-only {
+  #logo {
+    background-image: url(/logo.gif);
+  }
+}
+''';
+
+    var generated = r'''* html #logo {
+  background-image: url(/logo.gif);
+}''';
+
+    var stylesheet = compileCss(input, errors: errors, opts: options);
+
+    expect(stylesheet != null, true);
+    expect(errors.isEmpty, true, reason: errors.toString());
+    expect(prettyPrint(stylesheet), generated);
+  }
+}
+
+void mixin_content() {
+  void mixinArgs_var() {
+    final errors = [];
+    var input = r'''
+@mixin keyframes {
+  @-moz-keyframes    { @content; }
+  @-webkit-keyframes { @content; }
+}
+
+@include keyframes {
+   0%   { opacity: 0; }
+   100% { opacity: 1; }
+}
+''';
+
+    var generated = r'''@-moz-keyframes {
+  0%   { opacity: 0; }
+  100% { opacity: 1; }
+}
+
+@-webkit-keyframes {
+  0%   { opacity: 0; }
+  100% { opacity: 1; }
+}''';
+
+    var stylesheet = compileCss(input, errors: errors, opts: options);
+
+    expect(stylesheet != null, true);
+    expect(errors.isEmpty, true, reason: errors.toString());
+    expect(prettyPrint(stylesheet), generated);
+  }
+}
+
+void mixin_content_var() {
+  void mixinArgs_var() {
+    final errors = [];
+    var input = r'''
+@color: white;
+@mixin colors(@color: blue) {
+  background-color: var(color);
+  @content;
+  border-color: var(color);
+}
+.colors {
+  @include colors { color: var(color); }
+}
+''';
+
+    var generated = r'''.colors {
+  background-color: blue;
+  color: white;
+  border-color: blue;
+}
+''';
+
+    var stylesheet = compileCss(input, errors: errors, opts: options);
+
+    expect(stylesheet != null, true);
+    expect(errors.isEmpty, true, reason: errors.toString());
+    expect(prettyPrint(stylesheet), generated);
+  }
+}
+
+void badDeclarationInclude() {

[nweiz, 2013/09/18 22:40:54]

This shouldn't cause an error.

[terry, 2013/10/09 03:40:33]

Seems weird that top-level will close a declaration automatically.

span {
  border: 2px dashed red;
}
span #foo-id {
  color: red;
}

I'll consider matching this weird feature.

On 2013/09/18 22:40:54, nweiz wrote:

+  final errors = [];
+  final input = r'''
+@mixin a {
+  span {
+    border: 2px dashed red;
+    @include b;
+  }
+}
+
+@mixin b {
+  #foo-id {
+    color: red;
+  }
+}
+  ''';
+
+  var stylesheet = compileCss(input, errors: errors, opts: options);
+
+  expect(stylesheet != null, true);
+  expect(errors.isNotEmpty, true, reason: errors.toString());
+  expect(errors.length, 1, reason: errors.toString());
+  var error = errors[0];
+  expect(error.message,
+      'Trying to use top-level ruleset in a declaration - @include b');
+  expect(error.span.end.offset, 60);
+}
+
+void badTopInclude() {

[nweiz, 2013/09/18 22:40:54]

This *should* cause an error.

[terry, 2013/10/09 03:40:33]

Currently, we really report everything as a warning and recover.
On 2013/09/18 22:40:54, nweiz wrote:

+  final errors = [];
+  final input = r'''
+@mixin a {
+  span {
+    border: 2px dashed red;
+  }
+  @include b;
+}
+
+@mixin b {
+  color: red;
+}
+
+@include a;
+  ''';
+
+  var generated = r'''span {
+  border: 2px dashed #f00;
+}''';
+
+  var stylesheet = compileCss(input, errors: errors, opts: options);
+  expect(stylesheet != null, true);
+  expect(errors.isEmpty, true, reason: errors.toString());
+  expect(prettyPrint(stylesheet), generated);
+}
+
+void cycleDetect() {
+  final errors = [];
+  final input = r'''
+@mixin a {
+  @include b;
+}
+@mixin b {
+  span {
+    border: 2px dashed red;
+  }
+  @include a;
+}
+@include b;
+  ''';
+
+  var stylesheet = compileCss(input, errors: errors, opts: options);
+
+  expect(stylesheet != null, true);
+  expect(errors.isNotEmpty, true, reason: errors.toString());

[nweiz, 2013/09/18 22:40:54]

expect(errors, isNotEmpty)

[reason] here doesn't do anything, since it'll only be used when errors is empty
anyway.

[terry, 2013/10/09 03:40:33]

test is gone.
On 2013/09/18 22:40:54, nweiz wrote:

+  expect(errors.length, 2, reason: errors.toString());
+  var error = errors[0];
+  expect(error.message, 'mixin cycle detected @mixin a');

[nweiz, 2013/09/18 22:40:54]

Test the type of the error, not the message. Testing the message couples the
test to the specific error text used.

+  expect(error.span.end.offset, 24);
+  error = errors[1];
+  expect(error.message, 'mixin cycle detected @mixin b');
+  expect(error.span.end.offset, 92);
+}
+
+void cycleNesting() {
+  final errors = [];
+  final input = r'''
+@mixin a {
+  @include b;
+}
+@mixin b {
+  border: 2px dashed red;
+  @include a;
+}
+div {
+  @include b;
+}
+  ''';
+
+  var stylesheet = compileCss(input, errors: errors, opts: options);
+
+  expect(stylesheet != null, true);
+  expect(errors.isNotEmpty, true, reason: errors.toString());
+  expect(errors.length, 2, reason: errors.toString());
+  var error = errors[0];
+  expect(error.message, 'mixin cycle detected @mixin a');
+  expect(error.span.end.offset, 24);
+  error = errors[1];
+  expect(error.message, 'mixin cycle detected @mixin b');
+  expect(error.span.end.offset, 77);
+}
+
+main() {
+  test('top-level mixin', topLevelMixin);
+  test('top-level mixin 2 @includes', topLevelMixin2Includes);

[nweiz, 2013/09/18 22:40:54]

"2" -> "two", here and elsewhere. Also, at least use sentence fragments for test
descriptions (e.g. "top-level mixin with two @includes").

[terry, 2013/10/09 03:40:33]

Done.

+  test('top-level multi rulesets', topLevelMixinMultiRulesets);
+  test('top-level multi rulesets and nesting', topLevelMixinMultiRulesets2);
+  test('top-level selector groups', topLevelMixinSelectors);
+
+  test('declaration mixin', declSimpleMixin);
+  test('declaration mixin 2 @includes', declMixin2Includes);
+  test('declaration mixin includes', declMixinNestedIncludes);
+  test('declaration mixin includes #2', declMixinNestedIncludes2);

[nweiz, 2013/09/18 22:40:54]

Adding "#2" here doesn't tell me anything about the difference between the two
tests.  Be more descriptive in your test descriptions.

[terry, 2013/10/09 03:40:33]

Done.

+
+  test('detect cycle', cycleDetect);
+  test('detect cycle nesting', cycleNesting);
+  test('detect bad top-level as declaration', badDeclarationInclude);
+  test('detect bad declaration as top-level', badTopInclude);
+
+  // TODO(Terry): More tests to enable.

[nweiz, 2013/09/18 22:40:54]

Don't include tests that don't work. You can add these later along with the
feature that makes them work. This is what source control is for.

[terry, 2013/10/09 03:40:33]

Done.

+  // test('mixin arg', mixinArg);
+  // test('mixin args', mixinArgs);
+  // test('mixin args fanout', mixinArgs_2);
+  // test('mixin args with variables', mixinArgs_var);
+  // test('mixin content top-level', mixin_content_toplevel);
+  // test('mixin content', mixin_content);
+  // test('mixin content with variables', mixin_content_var);
+}