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

Active passing spec tests after PR 1249 #447

Merged
merged 2 commits into from
Jul 15, 2015

Conversation

@xzyfer
Copy link
Contributor

xzyfer commented Jul 14, 2015

Please pull out the failing tests for now. Otherwise 🚢

@mgreter
Copy link
Contributor Author

mgreter commented Jul 14, 2015

@xzyfer it seems that this CI runs against ruby sass!? That's why it is failing the 3 test cases we already discussed. Or am I seeing ghosts? Not sure how to handle this, but not having any specs because of minor discrepancies is the worst solution IMHO ... and I really don't want to replicate every "broken" behavior of ruby sass in libsass, because libsass IMO produces the correct output!

@xzyfer
Copy link
Contributor

xzyfer commented Jul 14, 2015

it seems that this CI runs against ruby sass

Yes this is on purpose. Spec should not fail on Ruby Sass, it would defeat the purpose of a Sass language spec if the reference implementation couldn't pass it.

That's why it is failing the 3 test cases we already discussed

We discussed leave the failing tests out for now whilst I chase up the Ruby team to address this on their end.

I really don't want to replicate every "broken" behavior of ruby sass in libsass

I understand, but Sass is about the Sass language, not LibSass. The goal is for other Sass implementors to be able to run their Sass implementation against sass spec to verify "correctness". Correct is currently defined as doing what Ruby Sass does.

libsass IMO produces the correct output!

I also agree but we can't change a spec's output for every future implementation's quirks.


The long term solution here is to change Sass spec from exact string comparison (it's already does some whitespace normalisation so it's "exact") to semantic comparisons on the output CSS's AST. This has been discuss with the Ruby Sass team and their onboard, it just requires resources.


As discussed in Slack, lets leave the failing tests out. We'll try to solve this issues for good.

@xzyfer xzyfer closed this Jul 14, 2015
@xzyfer xzyfer reopened this Jul 14, 2015
@xzyfer
Copy link
Contributor

xzyfer commented Jul 14, 2015

I keep hitting close instead of comment. I have butter fingers today.

@mgreter
Copy link
Contributor Author

mgreter commented Jul 14, 2015

In this case we should change ruby spec runner to "normalize" /,\s*/ to , in compressed mode ...

@xzyfer
Copy link
Contributor

xzyfer commented Jul 14, 2015

It's worth a shot. We need to be really careful with naive replacements so we don't mess with quoted strings and such.

To be safe we need to compare AST. I'm trying to get someone from the community to maintain Sass spec and take on these features/issues.

@mgreter mgreter force-pushed the activate/after-pr-1249 branch from a313d32 to e6a1b61 Compare July 15, 2015 19:20
@mgreter
Copy link
Contributor Author

mgreter commented Jul 15, 2015

I just saw that I already implemented something for this case (and it seems I also did it for the ruby spec runner). You actually can add an empty file named expected_outstyle.clean to activate the previousely used white-space cleaning. Looks like we can activate those tests afterall :smile;

@mgreter mgreter force-pushed the activate/after-pr-1249 branch from 90c1bfb to b42fe29 Compare July 15, 2015 19:35
mgreter added a commit that referenced this pull request Jul 15, 2015
Active passing spec tests after PR 1249
@mgreter mgreter merged commit b37420e into sass:master Jul 15, 2015
@mgreter mgreter deleted the activate/after-pr-1249 branch April 22, 2016 19:14
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.

2 participants