Change format of tests

[michaek]

Currently, every test requires its own directory, with two specifically named files: input.scss and expected_output.css. Because there are a lot of files, this makes reading through the tests and even grouping contextually related tests difficult.

I'd like to suggest some changes that would allow us to name files after their tests, and express multiple assertions per file. We could take our current extend-tests/001_test_basic and put the following in extend-tests/basic.scss:

/*
.foo, .bar {
  a: b; }
*/

.foo {a: b}
.bar {@extend .foo}

Because block comments are preserved in the output, we can compare the output from our scss to the contents of the comment block after compiling. It would be pretty easy to include multiple scss/comment pairs:

/*
.foo, .bar {
  a: b; }
*/

.foo {a: b;}
.bar {@extend .foo;}

/*
.foo, .bar {
  a: b; }
*/

.bar {@extend .foo;}
.foo {a: b;}

I think this would aid comprehensibility, and keep the parts of sass-spec that were derived from Ruby Sass's test cases closer to the structure of those tests. Thoughts?

[nschonni]

This made me think of something else (or remember), which would be a YAML or JSON test config file in each directory. The advantage to that is that the input, output, includes, or CLI parameters could be defined outside the harness files.

EX: test.json

{
  "input": "input.scss",
  "include": ["import1.scss", "import2.scss"],
  "output": "output.css",
  "version": ">3.2.2"
}

[michaek]

I like this in terms of organization when we're talking about a small number of tests, but I'd like to make it easier to understand at a glance what's going on with a test, and I think having everything in separate directories, and input/output in separate files is a barrier to understanding that we don't really need.

I'd prefer a convention over configuration approach, too, because things are more likely not to change between tests than to change. A test.json file makes things explicit, but there are approximately 700 tests, so this would mean another ~700 files that a newcomer would have to wrap her/his head around.

[chriseppstein]

In the example you gave above, you'd have to split the file up and feed it to the preprocessor as separate files in order to keep the tests from affecting each other. This makes the test infrastructure more complicated and the errors a bit more difficult to understand. I think you'd need to have more structure. E.g.

// ### Test: Simple extend of a class
// ### Source:
.foo {a: b;}
.bar {@extend .foo;}
// ### Expect:
.foo, .bar {
  a: b; }

// ### Test: Simple extend of a class further in document.
// ### Source:
.bar {@extend .foo;}
.foo {a: b;}
// ### Expect:
.foo, .bar {
  a: b; }

// ### Test: Missing curly brace raises an error.
// ### Source:
.foo { color: red;
// ### Error on Line 1:
Invalid CSS after ".foo { color: red;": expected "}", was ""

[nschonni]

I'd prefer a convention over configuration approach

:100: I think the test.yml/json would only be added for those tests that need to override the convention of input.scss and expected_output.css.

• Testing API/CLI options like precision
• Version or engine exclusion
• Expected error message rather than an output

[michaek]

@chriseppstein I agree - splitting the input for separate execution is better, and once we're splitting the input there's no need to "hide" the expected output in comments. You're right that this level of abstraction/indirection makes errors harder to understand.

@nschonni I like that. What if the YAML was included in the top of the file? I think it would probably go within a comment block, to keep the test files valid scss.

It's possible that I'm trying to solve a problem that's not as important as I think. It's been hard for me (and other members of my team) to read through the tests and get a sense of what's covered. But that doesn't mean that we should privilege human-readability over other requirements, just because my bias is to want to be able to read the tests clearly.

[nschonni]

@michaek I think YAML inside the input.scss might work. Only downside I can see is passing the file to a YAML parser which might not be happy with the SCSS content.

Alternately, maybe look at what https://github.com/SassDoc/sassdoc has to offer. But I'm just bikeshedding, so PR whatever makes sense :wink:

[michaek]

@nschonni I don't think you're bikeshedding - input on what's pleasing to people is just as valuable as input on what works. It only becomes bikeshedding when we can't escape a feedback cycle of insistent and mutually exclusive preferences. :)

I like the idea of using an approach consistent with SassDoc, especially if it becomes widely adopted. I'll take a look and see if there's a pattern that'll fit our needs.

[michaek]

SassDoc's comment parsing is via https://github.com/SassDoc/scss-comment-parser, which seems pretty cool, but to take advantage of it, we'd need to introduce nodejs. That might be worthwhile, if it helps significantly, but I wouldn't want to just start introducing languages into an exclusively Ruby project. :)

[michaek]

While SassDoc is written in JavaScript, Hologram is written in Ruby.

[mirisuzanne]

I like this proposal a lot (adjusted as necessary for easy parsing). These changes would fit well with a re-considered file organization, as discussed in #82.

[saper]

I am late to the party and from the view of the newcomer I'd like to strongly support status quo. Why?

Over the past two days, with practically zero Ruby experience, I was able to contribute larger things like #494 or tiny like #503. I really needed 15 minutes to get some little changes working and Ruby coding started to fell natural - because our code base is so tiny and easy to understand.

Plugging additional stuff like error or status, while maybe not winning beauty contests, proved simple and quickly lead to practical results like #502, which demonstrated few regressions or unfixed bugs in libsass.

Of course, one can argue that if we switch to JSON/YAML/whatever the code grows only by one or two lines where the file is read which just (de)serialize the file into ruby objects. But here is why I think this is not good:

Already in at least two cases (out of 14 I was working on) when testing for error output I have deployed hex dump to figure out WHY the messages returned by libsass and ruby sass differ. Was it additional space? A quote I am missing? Was one giving CR LF and the other LF? Tabs vs. spaces?

In those cases I just launch hexdump or some binary diff tools - call me weird. And I need plain old text files for that.

Stuffing it into JSON/YAML/whatever will cause the need to quote stuff to fit in the file and although I think the tools to do that are robust in the mean time I don't want to do an extra jq run just to extract the plain text value I want to have a detailed look at. This is also the reason why I didn't like having the error message embedded in the resulting CSS, the feature which I have changed in https://github.com/sass/sass-spec/pull/498.

And things like #1 an be fixed with a simple additional file that lists per-spec keywords, which are interpreted by Ruby classes (something like annotations). I'd go for the simplest words, even without parameters, like since_3.4 or whatever. And those rules should only feed the decision whether to skip or not the test.

[chriseppstein]

There is now a options.yaml file that stores metadata on a per-folder basis. These options are inherited by subdirectories. multiple files are being used to store expected output, but it's exceedingly easy now to generate and update these files using the sass-spec.rb --interactive mode or by using the --generate or --migrate options.