Run todo tests by default for CI

[mgreter]

IMO it would be nice to also check the todo tests on CI. We should not alter the overall error state for these, but it should report the spec tests that are unexpectedly passing. We sometimes add todo tests for segfaults, which should not be included here, since it would abort the whole test runner. Therefore I propose to add tests for segfaults to another folder, that would be excluded from this proposed test run.

[xzyfer]

Sass spec doesn't exists to serve LibSass, it's for all implementors of the Sass language. We should endeavour to remove LibSass specific logic not add more.

Testing for segfaults and such is something we should probably do in LibSass via a different method. I know @am11 has thoughts on this.

[am11]

Copy/paste from slack chat:

We can test Unix Signals (including sigsegv ) with Criterion. Here is a sample: https://github.com/Snaipe/Criterion/blob/ec6ea15/samples/signal.c. 😄

[xzyfer]

I think this is something we should move into LibSass. As well as testing the C-API.

[mgreter]

@xzyfer I mean todo specs that make the libsass master segfault (which we have had in the past). It's pretty inconvenient when I try to find todo specs that are passing with master automatically. I always have to move tests that segfault first and then make sure I don't accidentally commit it. I know it's not ideal, but we don't live in an ideal world. So I still would like to add spec tests that currently segfault for master to a libsass-todo-segfaults folder (instead of libsass-todo-issues). IMO this shouldn't change anything for ruby spec runner, it would allow me and others (including the CI test runner) to differ between segfaulting todo tests and ones I can automatically check.

[xzyfer]

@mgreter I understand now. I personally have not run into this. It seems worth doing if someone has the time and desire.

[saper]

@mgreter one question: if you have segfault tests in, say, libsass-todo-issues, what happens if you run sass-spec.rb against them without --ignore-todo ? You probably don't need to move them away?

Do I understand it correctly that you basically need to run sass-spec.rb --unexpected-pass ? In this case we should fix #333 - by either applying #508 or splitting tests that have some formats passing and some failing into separate sub-tests with one format per directory only.

[mgreter]

@saper don't really understand the relation to the issues you linked. But yes, the problem exists when we have segfaulting todo tests and run --unexpected-pass (in perl-libsass I unset skip-todo-tests).

@xzyfer I don't think we need to do much here. I just suggested to add any future todo specs that would segfault with libsass master into another folder. This way we could savely run --unexpected-pass with CI, which would conveniently show any newly passing spec tests for pending PRs (which was the main request for this ticket). This differentiation would need to be implemented into each spec runner!

If we enable this in CI, we need to ensure we don't run any spec tests that would abort the runner. Not sure our spec runner in CI would have a problem with that, but the perl-libsass spec runner would have a problem with that, since it doesn't spawn a new process for each test run.

[saper]

Regarding #333: there are "todo" tests that fail both when run normally and when run with --unexpected-pass, for example spec/libsass-todo-tests/libsass/propsets. So if we want to run --unexpected-pass automatically on all failing tests, we should address this issue (by splitting tests or by applying some fix, like #508 or similar).

[saper]

So the problem is really that you'd like to have a way to avoid crashing in-process testing when using perl-libsass? ("Fast" sass-spec.rb mode - without -c is also running in a single ruby process).

[saper]

I haven't seen crashing tests yet (crashing sassc normally shouldn't abort ruby), but for example in the "fast mode" we cannot run tests in parallel, because standard error output cannot be reliably captured when the tests go parallel (this is #499).

[mgreter]

Yep, because that would make it nearly impossible to activate this by default for CI, which was the main intend for this issue (if that applies to our spec runner). So the spec runner should really just report unexpectecly passing tests. I hope ruby spec runner does not error if some tests unexpectedly pass!? The main intention was really just to be able to easily check if a PR fixes any todo tests.
I redirect stderr to specific files when I run the tasks in parallel and "normalize" them afterwards.
BTW. something in sass-spec broke libsass master!

[saper]

We have to be consistent in what we are doing.

With --unexpected-pass we say "If those todo tests fail, be silent (pass). If those todo tests start to pass, raise an error to inform the developer". For example:

This command means:

• "Please test all cases including those marked todo. If they fail, fail the test"

The result means:

• "Failure: in compressed format out output is different than Ruby's"

> ruby sass-spec.rb -c /home/saper/sw/sassc/bin/sassc spec/libsass-todo-tests/libsass/propsets
Recursively searching under directory 'spec/libsass-todo-tests/libsass/propsets' for test files to test '/home/saper/sw/sassc/bin/sassc' with.
sassc: 3.3.0-dirty
libsass: 3.3.2
sass2scss: 1.0.5
Run options: --seed 14200

# Running:

.F..

Finished in 0.064997s, 61.5415 runs/s, 338.4785 assertions/s.

  1) Failure:
SassSpec::Test#test__compressed_spec/libsass-todo-tests/libsass/propsets [/home/saper/sw/libsass/sass-spec/lib/sass_spec/test.rb:42]:
Expected did not match output.
--- expected
+++ actual
@@ -1 +1 @@
-"div{background-image:url(foo.png);background-position:50%}span{background-image:url(bar.png);background-position:100%}p{border-upper-left:2px;border-upper-right:3px}foo|div{color:red}div sp\\,#abcan{color:red}div sp\\  p,div sp\\  |q,#abcan p,#abcan |q{background:blue;color:\\hey;width:\\10 \\20 \\ }div sp\\  p a,div sp\\  |q a,#abcan p a,#abcan |q a{height:1}d\\ v a,sp\\ n a{color:blue}"
+"div{background-image:url(foo.png);background-position:50%}span{background-image:url(bar.png);background-position:100%}p{border-upper-left:2px;border-upper-right:3px}foo|div{color:red}div sp\\ ,#abcan{color:red}div sp\\  p,div sp\\  |q,#abcan p,#abcan |q{background:blue;color:\\hey;width:\\10 \\20 \\ }div sp\\  p a,div sp\\  |q a,#abcan p a,#abcan |q a{height:1}d\\ v a,sp\\ n a{color:blue}"


4 runs, 22 assertions, 1 failures, 0 errors, 0 skips

This command means:

• "Please run all cases, if you find a test marked todo, and it still fails, be quiet (pass). If the test marked todo runs correctly, alert the developer (error)"

The result means:

• "Attention! This test actually passes for three formats! Maybe you can remove the todo mark?"

ruby sass-spec.rb -c /home/saper/sw/sassc/bin/sassc --unexpected-pass spec/libsass-todo-tests/libsass/propsets
Recursively searching under directory 'spec/libsass-todo-tests/libsass/propsets' for test files to test '/home/saper/sw/sassc/bin/sassc' with.
sassc: 3.3.0-dirty
libsass: 3.3.2
sass2scss: 1.0.5
Run options: --seed 15846

# Running:

EE.E

Finished in 0.074789s, 53.4838 runs/s, 267.4188 assertions/s.

  1) Error:
SassSpec::Test#test__compact_spec/libsass-todo-tests/libsass/propsets:
RuntimeError: /home/saper/sw/sass-spec/spec/libsass-todo-tests/libsass/propsets/input.scss passed a test we expected it to fail
    /home/saper/sw/libsass/sass-spec/lib/sass_spec/test.rb:61:in `run_spec_test'
    /home/saper/sw/libsass/sass-spec/lib/sass_spec/test.rb:74:in `block (2 levels) in create_tests'


  2) Error:
SassSpec::Test#test__nested_spec/libsass-todo-tests/libsass/propsets:
RuntimeError: /home/saper/sw/sass-spec/spec/libsass-todo-tests/libsass/propsets/input.scss passed a test we expected it to fail
    /home/saper/sw/libsass/sass-spec/lib/sass_spec/test.rb:61:in `run_spec_test'
    /home/saper/sw/libsass/sass-spec/lib/sass_spec/test.rb:74:in `block (2 levels) in create_tests'


  3) Error:
SassSpec::Test#test__expanded_spec/libsass-todo-tests/libsass/propsets:
RuntimeError: /home/saper/sw/sass-spec/spec/libsass-todo-tests/libsass/propsets/input.scss passed a test we expected it to fail
    /home/saper/sw/libsass/sass-spec/lib/sass_spec/test.rb:61:in `run_spec_test'
    /home/saper/sw/libsass/sass-spec/lib/sass_spec/test.rb:74:in `block (2 levels) in create_tests'

4 runs, 20 assertions, 0 failures, 3 errors, 0 skips

You have to invert the logic to alert the developer and return failing error code (prevent deployment for example). "Just reporting" is useless for CI - the developer will never look at the output if it's all "green".

[mgreter]

What I want is to be able to look myself at the CI result if a PR fixes a certain todo, that's my usecase. Would be a bit easier than having to create a PR to activate the spec test and it would potentially also catch any other todo tests I didn't anticipate. This is what I would like to request for this issue. Could very well also be a new cli option, like report-passing-todos ...

[saper]

My proposal would be the following:

• Fix The --unexpected-pass should take into account output styles #333 (by splitting 3 partial tests)
• Implement --only-todo option to save time and run only todo tests
• Have two scripts invoked via CI:
  • spec-sass.rb --ignore-todo -c sassc spec (normal run for regressions)
  • spec-sass.rb --only-todo --unexpected-pass -c sassc spec (run for positive regressions).
• The status code returned should be a conjunction of both tests

This way we get a red build whever something needs developer attention.