From 96f8b09cdcb14ac004be8792c5423560ac46dfa3 Mon Sep 17 00:00:00 2001 From: Natalie Weizenbaum Date: Mon, 18 Mar 2019 18:33:59 -0700 Subject: [PATCH] Add a style guide for writing new specs I've tried to capture the best practices I've come up with while writing tests in the HRX format, mixed with best practices from other testing traditions and a few rules that mostly serve to just establish a consistent style. --- STYLE_GUIDE.md | 1245 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 1245 insertions(+) create mode 100644 STYLE_GUIDE.md diff --git a/STYLE_GUIDE.md b/STYLE_GUIDE.md new file mode 100644 index 0000000000..0be2af534d --- /dev/null +++ b/STYLE_GUIDE.md @@ -0,0 +1,1245 @@ +# Style Guide + +Having a consistent style across all specs helps make them easier for a reader +to understand at a glance and easier for an author to focus on the behavior +being tested rather than the details of how the test is written. This guide +sets out the standard style that should be used for all new specs. + +Note that specs written before this style guide probably don't follow it exactly +(or at all). If you're inspired to fix up old specs, you're very welcome to do +so! + +Each guideline starts with one of these words: + +* **DO** guidelines describe practices that should always be followed. There + will almost never be a valid reason to stray from them. + +* **DON'T** guidelines are the converse: things that are almost never a good + idea. + +* **PREFER** guidelines are practices that you should follow. However, there may + be circumstances where it makes sense to do otherwise. Just make sure you + understand the full implications of ignoring the guideline when you do. + +* **AVOID** guidelines are the dual to "prefer": stuff you shouldn't do but + where there may be good reasons to on rare occasions. + +* **CONSIDER** guidelines are practices that you might or might not want to + follow, depending on circumstances, precedents, and your own preference. + +That specs that are testing inputs that violate these guidelines are, of course, +an exception. Thorough testing trumps style. + +* [Organization](#organization) + * [DO write specs in HRX files](#do-write-specs-in-hrx-files) + * [DO use as few HRX files as possible for a given feature](#do-use-as-few-hrx-files-as-possible-for-a-given-feature) + * [DON'T have HRX files longer than 500 lines or so](#dont-have-hrx-files-longer-than-500-lines-or-so) + * [DO use descriptive paths](#do-use-descriptive-paths) + * [DO put error specs in a separate "error" directory](#do-put-error-specs-in-a-separate-error-directory) + * [PREFER underscore-separated paths](#prefer-underscore-separated-paths) +* [Testing](#testing) + * [DO test only one thing per spec](#do-test-only-one-thing-per-spec) + * [DO use type selectors as placeholders](#do-use-type-selectors-as-placeholders) + * [DO use descriptive names for multiple placeholders](#do-use-descriptive-names-for-multiple-placeholders) + * [DO use single-letter names for irrelevant placeholders](#do-use-single-letter-names-for-irrelevant-placeholders) + * [DO use style rules for expression-level tests](#do-use-style-rules-for-expression-level-tests) + * [PREFER making imported files partials](#prefer-making-imported-files-partials) + * [DO name single imported or used files "other"](#do-name-single-imported-or-used-files-other) + * [PREFER referring to issues when marking specs as `:todo`](#prefer-referring-to-issues-when-marking-specs-as-todo) +* [Documentation](#documentation) + * [CONSIDER adding a comment explaining your spec](#consider-adding-a-comment-explaining-your-spec) + * [CONSIDER adding README.md files to parent directories](#consider-adding-readme.md-files-to-parent-directories) + * [DON'T write HRX comments](#dont-write-hrx-comments) +* [Formatting](#formatting) + * [DO use comment dividers to separate specs within an HRX file](#do-use-comment-dividers-to-separate-specs-within-an-hrx-file) + * [DO order files consistently](#do-order-files-consistently) + * [DO use Dart Sass's output as the default](#do-use-dart-sasss-output-as-the-default) + * [DO end each file with a newline](#do-end-each-file-with-a-newline) + * [DO use two spaces for indentation](#do-use-two-spaces-for-indentation) + * [PREFER one-line rules when they contain a single child with no children](#prefer-one-line-rules-when-they-contain-a-single-child-with-no-children) + * [DON'T use one-line rules when they contain more than one child](#dont-use-one-line-rules-when-they-contain-more-than-one-child) + * [DON'T use one-line rules when they contain grandchildren](#dont-use-one-line-rules-when-they-contain-grandchildren) + +## Organization + +### DO write specs in HRX files + +
+Example + +#### Good + +`nesting.hrx` + +```hrx +<===> single/input.scss +a { + b {c: d} +} + +<===> single/output.css +a b { + c: d; +} +``` + +#### Bad + +`nesting/single/input.scss` + +```scss +a { + b {c: d} +} +``` + +`nesting/single/output.css` + +```css +a b { + c: d; +} +``` + +
+ +All specs should be written in HRX files unless they contain invalid UTF-8 +characters. HRX files make it easy for code reviewers to see the context of +specs they're reviewing. + +### DO use as few HRX files as possible for a given feature + +
+Example + +#### Good + +`nesting.hrx` + +```hrx +<===> single/input.scss +a { + b {c: d} +} + +<===> single/output.css +a b { + c: d; +} + +<===> +================================================================================ +<===> double/input.scss +a { + b, c {d: e} +} + +<===> double/output.css +a b, a c { + d: e; +} +``` + +#### Bad + +`nesting/single.hrx` + +```hrx +<===> input.scss +a { + b {c: d} +} + +<===> output.css +a b { + c: d; +} +``` + +`nesting/double.hrx` + +```hrx +<===> input.scss +a { + b, c {d: e} +} + +<===> output.css +a b, a c { + d: e; +} +``` + +
+ +Combining specs into as few HRX files as possible helps keep the repo clean and +easy to navigate, make it easier to see connections between related specs, and +encourages the creation of more smaller specs. + +Note that specs *should not* be combined for unrelated features, and that HRX +files should be split apart if they become too large (see below). + +### DON'T have HRX files longer than 500 lines or so + +Very large HRX files can be as difficult to naviate as hundreds of tiny files. +Once an HRX file gets larger than about 500 lines, it's probably time to split +it into multiple files. To do so: + +* Create a directory with the same name as the HRX files. +* Take all the top-level subdirectories in the HRX file and make them their own + HRX files. +* If this creates too many small HRX files, consider combining some related + files back into more coarse-grained sub-divisions. + +### DO use descriptive paths + +
+Example + +#### Good + +`css/keyframes.hrx` + +```hrx +<===> bubble/empty/input.scss +// Regression test for sass/dart-sass#611. +a { + @keyframes {/**/} +} + +<===> bubble/empty/output.css +@keyframes { + /**/ +} +``` + +#### Bad + +`regression/dart_sass_611.hrx` + +```hrx +<===> input.scss +a { + @keyframes {/**/} +} + +<===> output.css +@keyframes { + /**/ +} +``` + +
+ +The path to a spec, both inside and outside its HRX file, is the first place a +reader will look to try to understand [exactly what it's testing][]. Being as +descriptive as possible (without being too verbose) in your choice of names will +make this understanding much easier. Good rules of thumb include: + +[exactly what it's testing]: #do-test-only-one-thing-per-spec + +* Describe the feature or behavior being tested, not the circumstances around + how the test came to be created. + +* Use noun phrases (like "keyframes") or adjectives (like "empty") that describe + the feature being tested or the context or state it's in. + +* Avoid placeholder names like "basic" or "simple". Sometimes it's best to + describe *how* it's basic, like "empty" or "one_child". Other times, you can + just forego the "basic" directory entirely and put your specs beneath the + feature itself. + +If you can't fully and tersely describe a spec with its name alone, [add a +comment][] to the beginning of the Sass file. + +[add a comment]: #consider-adding-a-comment-explaining-your-spec + +### DO put error specs in a separate "error" directory + +
+Example + +#### Good + +```hrx +<===> single_value/input.scss +a { + --b: c; +} + +<===> single_value/output.css +a { + --b: c; +} + +<===> +================================================================================ +<===> error/empty/input.scss +// CSS requires at least one token in a custom property. +.empty { + --property:; +} + +<===> error/empty/error +Error: Expected token. + , +3 | --property:; + | ^ + ' + /sass/spec/css/custom_properties/error/empty/input.scss 3:14 root stylesheet +``` + +#### Bad + +```hrx +<===> single_value/input.scss +a { + --b: c; +} + +<===> single_value/output.css +a { + --b: c; +} + +<===> +================================================================================ +<===> empty/input.scss +// CSS requires at least one token in a custom property. +.empty { + --property:; +} + +<===> empty/error +Error: Expected token. + , +3 | --property:; + | ^ + ' + /sass/spec/css/custom_properties/error/empty/input.scss 3:14 root stylesheet +``` + +
+ +All error specs for a given feature should go in an `error` directory (which is +often its own HRX file) directly beneath the root directory for the feature +being tested. They should *not* be intermingled with the feature's success case +specs. This helps keep the success specs focused on demonstrating the expected +behavior of the feature, while the error tests can cover all the disallowed edge +cases without getting in the way. + +### PREFER underscore-separated paths + +
+Example + +#### Good + +`css/min_max.hrx` + +```hrx +<===> plain_css/nested_min_max/input.scss +a { + b: min(max(1px, 2px)) max(min(1px, 2px)); +} + +<===> plain_css/nested_min_max/output.css +a { + b: min(max(1px, 2px)) max(min(1px, 2px)); +} +``` + +L#### Bad + +`css/minMax.hrx` + +```hrx +<===> plain-css/nested-min-max/input.scss +a { + b: min(max(1px, 2px)) max(min(1px, 2px)); +} + +<===> plain-css/nested-min-max/output.css +a { + b: min(max(1px, 2px)) max(min(1px, 2px)); +} +``` + +
+ +Path names should be underscore-separated rather than hyphenated, +space-separated, or camel-cased. The only exception is when the path is +describing an existing Sass or CSS identifier that contains hyphens, such as +`font-face` or `scale-color`. + +## Testing + +### DO test only one thing per spec + +
+Example + +#### Good + +```hrx +<===> transparent/input.scss +a {b: hsl(180, 60%, 50%, 0)} + +<===> transparent/output.css +a { + b: rgba(51, 204, 204, 0); +} + +<===> +================================================================================ +<===> opaque/input.scss +a {b: hsl(180, 60%, 50%, 1)} + +<===> opaque/output.css +a { + b: #33cccc; +} +``` + +#### Bad + +```hrx +<===> input.scss +a { + transparent: hsl(180, 60%, 50%, 0); + opaque: hsl(180, 60%, 50%, 1); +} + +<===> output.css +a { + transparent: hsl(180, 60%, 50%, 1); + opaque: #33cccc; +} +``` + +
+ +Each individual spec should focus on exactly one assertion of the behavior in +question. It's worth a bit of extra verbosity to ensure that it's always clear +exactly what each spec is testing. Otherwise, it's extremely difficult to +refactor specs because you don't know which of the dozen behaviors might not be +tested anywhere else. It also ensures that an implementation can mark an +individual assertion as `:todo` without also ceasing to test a bunch of +unrelated behavior. + +### DO use type selectors as placeholders + +
+Example + +#### Good + +```hrx +<===> transparent/input.scss +a {b: hsl(180, 60%, 50%, 0)} + +<===> transparent/output.css +a { + b: rgba(51, 204, 204, 0); +} +``` + +#### Bad + +```hrx +<===> transparent/input.scss +.a {b: hsl(180, 60%, 50%, 0)} + +<===> transparent/output.css +.a { + b: rgba(51, 204, 204, 0); +} +``` + +
+ +When you need a style rule as context for something else, [like a test for an +expression][], the kind of selector you use for it doesn't matter. It's just +there as a placeholder to make the style rule valid. In that case, use a type +selector like `a` to avoid any unnecessary punctuation. + +[like a test for an expression]: #do-use-style-rules-for-expression-level-tests + +### DO use descriptive names for multiple placeholders + +
+Example + +#### Good + +```hrx +<===> input.scss +extendee {a: b} + +extender { + @extend extendee; +} + +<===> output.css +extendee, extender { + a: b; +} +``` + +#### Bad + +``` +<===> input.scss +a {b: c} + +d { + @extend a; +} + +<===> output.css +a, d { + b: c; +} +``` + +
+ +If you have more than one placeholder selector (or more than one placeholder +property, etc) in the same test, give them explicit names to help make it clear +how the input maps to the output. These names should describe the role of the +placeholders (or their associated rules), or at least their original locations. + +### DO use single-letter names for irrelevant placeholders + +
+Example + +#### Good + +```hrx +<===> input.scss +a {b: c} + +<===> output.css +a { + b: c; +} +``` + +#### Bad + +```hrx +<===> input.scss +@function global-function() {@return null} + +global-function-exists { + result: function-exists(global-function); +} + +<===> output.css +global-function-exists { + result: true; +} +``` + +
+ +If your placeholder identifiers *aren't* [repeated][], use single-letter names +in alphabetical order for them. The purpose of the spec should already be +communicated by [its path][], and it should only be testing [one thing][], so +there should be no need to add extra description to a single selector or +property name. + +[repeated]: #do-use-descriptive-names-for-multiple-placeholders +[its path]: #do-use-descriptive-paths +[one thing]: #do-test-only-one-thing-per-spec + +### DO use style rules for expression-level tests + +
+Example + +#### Good + +```hrx +<===> transparent/input.scss +a {b: hsl(180, 60%, 50%, 0)} + +<===> transparent/output.css +a { + b: rgba(51, 204, 204, 0); +} +``` + +#### Bad + +```hrx +<===> transparent/input.scss +@debug hsl(180, 60%, 50%, 0); + +<===> transparent/output.css +<===> transparent/warning +/spec/core_functions/hsl/transparent/input.scss:1 Debug: rgba(51, 204, 204, 0) +``` + +
+ +When you're testing something at the expression level, like a function or an +arithmetic operation, write the expression as the right-hand side of a +declaration in a style rule. This will produce as little visual noise as +possible without tying your spec to the implementation-specific formatting of +`@warn` or `@debug`. + +### PREFER making imported files partials + +
+Example + +#### Good + +```hrx +<===> member/variable/input.scss +@import "other"; + +a {b: $c} + +<===> member/variable/_other.scss +$c: d; + +<===> member/variable/output.scss +a { + b: d; +} +``` + +#### Bad + +```hrx +<===> member/variable/input.scss +@import "other"; + +a {b: $c} + +<===> member/variable/other.scss +$c: d; + +<===> member/variable/output.scss +a { + b: d; +} +``` + +
+ +When writing a spec with multiple files, make the files other than the import +partials unless there's a concrete reason not to. This helps make it clearer at +a glance that they're additional input files, rather than expectation files. + +### DO name single imported or used files "other" + +
+Example + +#### Good + +```hrx +<===> member/variable/input.scss +@import "other"; + +a {b: $c} + +<===> member/variable/_other.scss +$c: d; + +<===> member/variable/output.scss +a { + b: d; +} +``` + +#### Bad + +```hrx +<===> member/variable/input.scss +@import "imported"; + +a {b: $c} + +<===> member/variable/_imported.scss +$c: d; + +<===> member/variable/output.scss +a { + b: d; +} +``` + +
+ +When a spec involves exactly two files, the file that's not `input.scss` should +be named "other" (`_other.scss`, `_other.sass`, etc). + +### PREFER referring to issues when marking specs as `:todo` + +
+Example + +#### Good + +```hrx +<===> slash_slash_string/options.yml +--- +:todo: +- libsass # sass/libsass#2840 + +<===> slash_slash_string/input.scss +a {b: 1 / 2 / foo()} + +<===> slash_slash_string/output.css +a { + b: 1/2/foo(); +} +``` + +#### Bad + +```hrx +<===> slash_slash_string/options.yml +--- +:todo: +- libsass + +<===> slash_slash_string/input.scss +a {b: 1 / 2 / foo()} + +<===> slash_slash_string/output.css +a { + b: 1/2/foo(); +} +``` + +
+ +When [marking a spec as `:todo`][], follow it with a comment that refers to an +issue in the implementation in question tracking the feature the spec tests. If +there isn't an issue yet, create one. Make sure to also link that issue back to +the specs for it! + +[marking a spec as "todo"]: README.md#todo + +## Documentation +### CONSIDER adding a comment explaining your spec + +
+Example + +#### Good + +``` +<===> empty/input.scss +// CSS requires at least one token in a custom property. +.empty { + --property:; +} + +<===> empty/error +Error: Expected token. + , +3 | --property:; + | ^ + ' + /sass/spec/css/custom_properties/error/empty/input.scss 3:14 root stylesheet +``` + +
+ +If the details of what your spec is testing and why it produces the output it +does wouldn't be clear to someone moderately familiar with Sass from [the spec's +path][] and a casual reading of the spec's contents, it's a good idea to add a +comment explaining in detail what's going on. This can also be useful for +elucidating the stranger corners of the CSS spec or Sass's language semantics. + +[the spec's path]: #do-use-descriptive-paths + +Explanatory comments should always be silent Sass comments. Loud comments and +HRX comments must not be used. + +### DON'T write HRX comments + +
+Example + +#### Bad + +``` +<===> +CSS requires at least one token in a custom property. +<===> empty/input.scss +.empty { + --property:; +} + +<===> empty/error +Error: Expected token. + , +3 | --property:; + | ^ + ' + /sass/spec/css/custom_properties/error/empty/input.scss 3:14 root stylesheet +``` + +
+ +HRX supports its own comment syntax, but that should never be used for specs. +Instead [use Sass comments][] to document an individual file or spec and +[`README.md` files][] to document entire directories. + +[use Sass comments]: #consider-adding-a-comment-explaining-your-spec +[`README.md` files]: #consider-adding-readme-md-files-to-parent-directories + +### CONSIDER adding README.md files to parent directories + +
+Example + +#### Good + +```hrx +<===> color/literal/README.md +When colors are written literally by the user, rather than being generated by a +function, `inspect()` should return them in the same format the user wrote them. + +<===> +================================================================================ +<===> color/literal/short_hex/input.scss +a { + $result: inspect(#00f); + result: $result; + type: type-of($result); +} + +<===> color/literal/short_hex/output.css +a { + result: #00f; + type: string; +} + +<===> +================================================================================ +<===> color/literal/long_hex/input.scss +a { + $result: inspect(#0000ff); + result: $result; + type: type-of($result); +} + +<===> color/literal/long_hex/output.css +a { + result: #0000ff; + type: string; +} +``` + +
+ +Like [adding a comment][] to an individual spec, add a `README.md` file to a +directory to explain something about all the specs that directory contains. +There's not currently a way to render the Markdown, but it provides a consistent +format that's easy to read in plain text. + +[adding a comment]: #consider-adding-a-comment-explaining-your-spec + +## Formatting +### DO use comment dividers to separate specs within an HRX file + +
+Example + +#### Good + +```hrx +<===> single/input.scss +a { + b {c: d} +} + +<===> single/output.css +a b { + c: d; +} + +<===> +================================================================================ +<===> double/input.scss +a { + b, c {d: e} +} + +<===> double/output.css +a b, a c { + d: e; +} +``` + +#### Bad + +```hrx +<===> single/input.scss +a { + b {c: d} +} + +<===> single/output.css +a b { + c: d; +} + +<===> double/input.scss +a { + b, c {d: e} +} + +<===> double/output.css +a b, a c { + d: e; +} +``` + +
+ +The first file of each spec within an HRX file other than the first should begin with: + +```hrx +<===> +================================================================================ +``` + +This is an HRX comment, and has no effect on the semantics of the file. It just +serves to visually separate specs from one another. + +These comment separators should also be added before specs' parent directories +if they're added separately, such as for an `options.yml` or [a `README.md`][] +file. + +[a `README.md`]: #consider-adding-readme-md-files-to-parent-directories + +### DO order files consistently + +
+Example + +#### Good + +```hrx +<===> top_level/options.yml +--- +:warning_todo: +- libsass # sass/libsass#2834 +:ignore_for: +- ruby-sass + +<===> top_level/input.scss +$var: value !global; +a {b: $var} + +<===> top_level/output.css +a { + b: value; +} + +<===> top_level/warning +DEPRECATION WARNING: As of Dart Sass 2.0.0, !global assignments won't be able to +declare new variables. Consider adding `$var: null` at the top level. + + , +1 | $var: value !global; + | ^^^^^^^^^^^^^^^^^^^ + ' + /sass/spec/variables/global/first_declaration/top_level/input.scss 1:1 root stylesheet +``` + +#### Bad + +```hrx +<===> top_level/output.css +a { + b: value; +} + +<===> top_level/warning +DEPRECATION WARNING: As of Dart Sass 2.0.0, !global assignments won't be able to +declare new variables. Consider adding `$var: null` at the top level. + + , +1 | $var: value !global; + | ^^^^^^^^^^^^^^^^^^^ + ' + /sass/spec/variables/global/first_declaration/top_level/input.scss 1:1 root stylesheet + +<===> top_level/options.yml +--- +:warning_todo: +- libsass # sass/libsass#2834 +:ignore_for: +- ruby-sass + +<===> top_level/input.scss +$var: value !global; +a {b: $var} +``` +k +
+ +To keep specs consistent, their constituent files should always be written in +the following order: + +1. `options.yml`. +2. `input.scss` or `input.sass`. +3. Any files imported or used by `input.scss` or `input.sass`. +4. `output.css` or `error` +5. `warning` + +If a spec has [implementation-specific expectations][], those expectations +should go directly after the corresponding default expectation. + +[implementation-specific expectations]: README.md#implementation-specific-expectations + +### DO use Dart Sass's output as the default + +
+Example + +#### Good + +```hrx +<===> single_channel/input.scss +a {b: complement(#f00)} + +<===> single_channel/output.css +a { + b: aqua; +} + +<===> single_channel/output-libsass.css +a { + b: cyan; +} +``` + +#### Bad + +```hrx +<===> single_channel/input.scss +a {b: complement(#f00)} + +<===> single_channel/output.css +a { + b: cyan; +} + +<===> single_channel/output-dart-sass.css +a { + b: aqua; +} +``` + +
+ +Since Dart Sass is the reference implementation, its output should be used as +`output.css`, and other implementations' outputs (if they differ) should be +written as [implementation-specific expectations][]. + +### DO end each file with a newline + +
+Example + +#### Good + +```hrx +<===> single/input.scss +a { + b {c: d} +} + +<===> single/output.css +a b { + c: d; +} +``` + +#### Bad + +```hrx +<===> single/input.scss +a { + b {c: d} +} +<===> single/output.css +a b { + c: d; +} +``` + +
+ +Unless the lack of a trailing newline is specifically being tested, all +non-empty files should end in a trailing newline. For all but the last file in +an HRX archive, this appears as a full blank line. + +### DO use two spaces for indentation + +
+Example + +#### Good + +```hrx +<===> single/input.scss +a { + b {c: d} +} + +<===> single/output.css +a b { + c: d; +} +``` + +#### Bad + +```hrx +<===> single/input.scss +a { + b {c: d} +} + +<===> single/output.css +a b { + c: d; +} +``` + +
+ +Always use two spaces for indentation unless you're explicitly testing behavior +for other whitespace. + +### PREFER one-line rules when they contain a single child with no children + +
+Example + +#### Good + +```hrx +<===> single/input.scss +a { + b {c: d} +} + +<===> single/output.css +a b { + c: d; +} +``` + +#### Bad + +```hrx +<===> single/input.scss +a { + b { + c: d; + } +} + +<===> single/output.css +a b { + c: d; +} +``` + +
+ +When a rule contains a single child statement, such as a style rule that +contains a single declaration or a `@function` that just returns a value, it +should generally be written as a single line with no whitespace after `{` or +before `}`. However, if this would produce too long of a line or otherwise make +the rule difficult to read, it may be split onto multiple lines. + +### DON'T use one-line rules when they contain more than one child + +
+Example + +#### Good + +```hrx +<===> two_declarations/input.scss +a { + b: c; + d: e; +} + +<===> two_declarations/output.css +a { + b: c; + d: e; +} +``` + +#### Bad + +```hrx +<===> two_declarations/input.scss +a {b: c; d: e} + +<===> two_declarations/output.css +a { + b: c; + d: e; +} +``` + +
+ +When a rule contains multiple child statements, each child must be written on +its own line. Each child that doesn't take a block must be followed by a +semicolon. + +### DON'T use one-line rules when they contain grandchildren + +
+Example + +#### Good + +```hrx +<===> single/input.scss +a { + b {c: d} +} + +<===> single/output.css +a b { + c: d; +} +``` + +#### Bad + +```hrx +<===> single/input.scss +a {b {c: d}} + +<===> single/output.css +a b { + c: d; +} +``` + +
+ +If a rule contains another rule which itself has children, the grandparent +should never be written on a single line.