Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Test all output styles #270

Merged
merged 10 commits into from
Mar 12, 2015
Merged

Test all output styles #270

merged 10 commits into from
Mar 12, 2015

Conversation

mgreter
Copy link
Contributor

@mgreter mgreter commented Mar 1, 2015

Related to sass/libsass#910 and sass/sassc#97

  • Adds expected files for all output styles
  • Changes test runner to test all output styles
  • Added flag files to skip or clean certain tests

@xzyfer
Copy link
Contributor

xzyfer commented Mar 1, 2015

Could you make sure the nuke flag produces all these new spec file types? (it might already do so, it's hard to tell from the diff)

@xzyfer
Copy link
Contributor

xzyfer commented Mar 1, 2015

/cc @chriseppstein

@mgreter
Copy link
Contributor Author

mgreter commented Mar 2, 2015

By the way, quite a few todo tests emit this warning in ruby sass:

The subject selector operator "!" is deprecated and will be removed in a future release.
This operator has been replaced by ":has()" in the CSS spec.

I wonder if we want to update those tests and if we even want to implement the deprecated syntax?

@xzyfer
Copy link
Contributor

xzyfer commented Mar 3, 2015

We should continue to support those specs until Ruby Sass starts to throw an error. Other wise we're not a drop in replacement.

@xzyfer
Copy link
Contributor

xzyfer commented Mar 3, 2015

Also fixes #247

@mgreter
Copy link
Contributor Author

mgreter commented Mar 6, 2015

Added PR to my own branch for sass/libsass#922 => mgreter#1 😜

@mgreter
Copy link
Contributor Author

mgreter commented Mar 9, 2015

IMO we should get a decision if we want to merge this PR before releasing libsass 3.2!
Since it is a pretty big change I would like to have some feedback before merging it on my own!
//CC @chriseppstein @hcatlin @akhleung @xzyfer @am11

@xzyfer
Copy link
Contributor

xzyfer commented Mar 9, 2015

IMO this is definitely worth doing, especially if all these specs are currently passing on 3.2!

As long the utility methods (nuke, unexpected-pass) function correctly then this gets a 👍 from me.

The one possible concern could the significantly increased running time of Ruby Sass if they were to switch to sass-spec sometime in the future?

It's worth noting it's likely that sass-spec will undergo significant changes with @chriseppstein's work as per #82 (comment).

@am11
Copy link
Contributor

am11 commented Mar 9, 2015

👍
@mgreter, (at least on http://sassmeister.com/) it seems like ruby sass has default output style set to expanded. Should we do the same here (and then all the down-streams will follow)?

This (breaking) change will be the perfect candidate for forthcoming major version of node-sass.

//cc @jedfoster.

@mgreter
Copy link
Contributor Author

mgreter commented Mar 9, 2015

IMO the nuke option etc. should still work, since it only adds more tests with different output options and expected output path. But I actually never did any ruby before these changes to the spec runner. So I really would like to get a O.K. from someone that knows ruby.

The running time to run the spec test might be an issue, but IMO it's worth it. My perl-libsass spec runner needs 15s-20s to do all 4044 checks (including todo test). Regenerating all 4044 spec test via ruby sass takes about 3 minutes, while exhausting all my 4 cores (i7-3770).

It will probably also increase the coverage by one or two percent (I would guess)!
And I've kept the old format for the nested output style, so old runners should still work!

@mgreter mgreter mentioned this pull request Mar 11, 2015
mgreter added 8 commits March 12, 2015 02:38
They contain a lot of edge cases in a pretty brute-force
format. Very helpful to discover subtle differences between
ruby sass and libsass. Mostly tests interpolation and
specially escaped sequences and other format issues.
Reverts the test to the old behavior, where white-
space is completely stripped down and not compared.
The are probably hand written and were never generated.
They failed with the new more white space sensitive test.
We now test all four output styles with strict white-space
rules. We only normalize multiple line-feeds, which we hope
to solve with one of the next libsass releases!
A few tests exhibit very strange minimal differences.
@mgreter mgreter force-pushed the feature/output-styles branch from 60b9a1e to 8b2245b Compare March 12, 2015 02:03
mgreter added 2 commits March 12, 2015 03:14
Do a much more white-space sensitive compare.
Does only need to normalize multiple line-feeds.
mgreter added a commit that referenced this pull request Mar 12, 2015
Test all output styles (more white-space sensitive)
@mgreter mgreter merged commit 4b5b1cf into sass:master Mar 12, 2015
@mgreter mgreter deleted the feature/output-styles branch April 22, 2016 19:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants