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

Add sass tests too #842

Merged
merged 2 commits into from
Dec 29, 2021
Merged

Add sass tests too #842

merged 2 commits into from
Dec 29, 2021

Conversation

XhmikosR
Copy link
Contributor

@XhmikosR XhmikosR commented Dec 29, 2021

@xzyfer I think it's worth having this. That being said, I noticed a few differences while working on this patch. This one makes me wonder:

https://github.com/dlmanning/gulp-sass/pull/842/files#diff-5a684c173d69bcc8d9e0e4b50ac83d8eaf640dbd8e141c14d9e1a953c87afbd3R482

@XhmikosR
Copy link
Contributor Author

Oh, and if you have another idea how to implement this; I went with the environment variable solution.

@@ -1,5 +1,5 @@
$blue: #3bbfce;
$margin: 16px;
$blue: #3bbfce
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dart sass threw an error for indented syntax with semicolons

stream.on('data', (cssFile) => {
assert.ok(cssFile.sourceMap);
assert.deepEqual(cssFile.sourceMap.sources, expectedSources);
assert.deepEqual(cssFile.sourceMap.sources.sort(), expectedSources.sort());
Copy link
Contributor Author

@XhmikosR XhmikosR Dec 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to use sort() since dart sass and node-sass produce arrays with different sorting.

@XhmikosR XhmikosR marked this pull request as ready for review December 29, 2021 08:10
test/main.js Outdated Show resolved Hide resolved
@@ -458,19 +479,21 @@ describe('gulp-sass -- sync compile', () => {
'inheritance.scss',
];

if (COMPILER === 'sass') expectedSourcesAfter.push('inheritance.css');
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes me wonder if something else is wrong... Dart sass includes the dist file while node-sass doesn't.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Source maps are not part of the sass language spec and there are difference between engines. It's likely node-sass is wrong in this case. A more resilient approach might be to check for the intersection of the sources rather than a matching setting.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel free to push your suggestion if you want otherwise let's land it as is.

@@ -157,13 +162,17 @@ describe('gulp-sass -- async compile', () => {
const stream = sass();

stream.on('error', (err) => {
// Error must include message body
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use this style because error message formats are not part of the sass language spec and change over time. This style is more resilient to compiler changes as it simple asserts for key phrases.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, but it's more troublesome to keep them in sync. For example, how should I test for the line with dart sass? Just check for number 2?

Copy link
Collaborator

@xzyfer xzyfer Dec 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd opt for simply removing the line number assertion. This is more the concern of the engine. All we really care about is that an error was produced.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check now, it should be what you want. The line is still checked in another test, not sure if we should remove that too.

@@ -10,7 +10,9 @@
"lint": "eslint --report-unused-disable-directives --ignore-path .gitignore .",
"fix": "npm run lint -- --fix",
"mocha": "mocha",
"test": "mocha"
"test": "npm run test:node-sass && npm run test:dart-sass",
"test:node-sass": "mocha",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me know if you want different script names here.

package.json Outdated
"test": "mocha"
"test": "npm run test:node-sass && npm run test:dart-sass",
"test:node-sass": "mocha",
"test:dart-sass": "cross-env SASS_COMPILER=sass mocha"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity could you do something like mocha -- sass and pull the compiler off of argv instead of introducing cross-env?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could probably work (not mocha -- sass but mocha -- --sass, but seems more hacky from a quick look. Feel free to tweak it, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, wait. Let me try something.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed a patch which does work but still seems hacky, but correct me if I'm wrong. Aren't we abusing the fact that mocha doesn't throw on unknown command line options?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with it until it's a problem :)

@xzyfer xzyfer merged commit 835d6f3 into dlmanning:master Dec 29, 2021
@XhmikosR XhmikosR deleted the dart-sass-tests branch December 29, 2021 09:19
@XhmikosR
Copy link
Contributor Author

XhmikosR commented Dec 29, 2021

@xzyfer how about releasing a new minor version? We've landed plenty of changes and personally I don't have any further ones. :)

EDIT: apart from #286 (comment) but I'm not sure it's still an issue anyway.

@xzyfer
Copy link
Collaborator

xzyfer commented Dec 31, 2021

v5.1.0 has been released

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