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 test cases for sass and include necessary sass test files #743

Merged

Conversation

saptar
Copy link

@saptar saptar commented Jun 17, 2016

This PR is an extension to PR #723 which is already merged.

This PR brings in additional tests to cover .sass syntax.

Tries to Resolve issue #70

DCO 1.1 Signed-off-by: Saptarshi Dutta saptarshidutta31@yahoo.co.in

DCO 1.1 Signed-off-by: Saptarshi Dutta <saptarshidutta31@yahoo.co.in>
@coveralls
Copy link

coveralls commented Jun 17, 2016

Coverage Status

Coverage remained the same at 97.237% when pulling 26eef9f on saptar:feature/disable-linters into c9a3b65 on sasstools:feature/disable-linters.

@saptar saptar mentioned this pull request Jun 17, 2016
@DanPurdy
Copy link
Member

Thanks @saptar one of the tests here is failing. We'll have to investigate it.

@ackl
Copy link

ackl commented Jun 21, 2016

FWIW the test that fails on appveyor is fine when run locally on my machine, tested with node v4 v5 & v6

@DanPurdy
Copy link
Member

DanPurdy commented Jun 21, 2016

@ackl are you on windows using CRLF line endings? Appveyor is testing that scenario.

@ackl
Copy link

ackl commented Jun 21, 2016

@DanPurdy
No I'm not using windows/CRLF line endings. That is indeed the cause of the test failure

I converted to CRLF in the relevant sass file
$ unix2dos tests/sass/ruleToggler-disable-a-block.sass
and the test fails.

The test is expecting end of block column to be column 32, but due to CRLF line ending it returns 33 on windows (lib/ruleToggler L:21).

I'm guessing this should be fixed in gonzales? I can reproduce similar issue:

> sass = 'p\n  // sass-lint:disable-block\n'
'p\n  // sass-lint:disable-block\n'
> sassCRLF = 'p\r\n  // sass-lint:disable-block\r\n'
'p\r\n  // sass-lint:disable-block\r\n'
> parseTree = gonzales.parse(sass, {syntax: 'sass'})
Node {
  type: 'stylesheet',
  content:
   [ Node {
       type: 'ruleset',
       content: [Object],
       syntax: 'sass',
       start: [Object],
       end: [Object] },
     Node {
       type: 'space',
       content: '\n',
       syntax: 'sass',
       start: [Object],
       end: [Object] } ],
  syntax: 'sass',
  start: { line: 1, column: 1 },
  end: { line: 2, column: 29 } }
> parseTreeCRLF = gonzales.parse(sassCRLF, {syntax: 'sass'})
Node {
  type: 'stylesheet',
  content:
   [ Node {
       type: 'ruleset',
       content: [Object],
       syntax: 'sass',
       start: [Object],
       end: [Object] },
     Node {
       type: 'space',
       content: '\n',
       syntax: 'sass',
       start: [Object],
       end: [Object] } ],
  syntax: 'sass',
  start: { line: 1, column: 1 },
  end: { line: 2, column: 30 } }

@DanPurdy
Copy link
Member

DanPurdy commented Jun 22, 2016

Yep, as assumed 😭

I'll take a look at gonzales again, thought i'd fixed all the CRLF issues..

It puzzles me looking at that though why we don't see the same issue in scss format. Let me double check because I assume the '\r\n' is counting as 2 columns length throughout gonzales when it should probably only be 1 (being a newline character)

Thanks for taking the time to look further into it @ackl

If anyone is interested the CRLF / LF checks are here https://github.com/tonyganch/gonzales-pe/blob/dev/src/sass/tokenizer.js#L345

It may not be related as I had tests to check column counts in gonzales but It's definitely worth a look.

@ackl
Copy link

ackl commented Jun 23, 2016

Did some more digging around, I have a feeling this stems from the way gonzales handles the end of BlockType Nodes.

The ruleToggler tests use the ruleToggler.getToggledRules(lib/ruleToggler.js L:64) method which in turn calls traverseByTypes on the gonzales ast.

  ast.traverseByTypes(['multilineComment', 'singlelineComment'], function (comment, i, parent) {...}

(lib/ruleToggler.js L:74)

Note the parent arg in the callback - in the failing test, this is the BlockType Node which is used by the addDisabledBlock function to retrieve start/end line/columns.

Thus CRLF after the closing brace on a block in scss files does not adversely affect the Node.end.column value which we check for in the tests.

I guess for sass files, with no closing brace to indicate the end of a BlockType Node, gonzales will take the end of line as the end of the block thus including the \r\n chars?

heres some code to illustrate what I mean:

let gonzales = require('gonzales-pe');
let scssFilePath = 'path/to/sass-lint/tests/sass/ruleToggler-disable-a-block.scss';
let sassFilePath = 'path/to/sass-lint/tests/sass/ruleToggler-disable-a-block.sass';

let scssAST = gonzales.parse(fs.readFileSync(scssFilePath).toString(), {syntax: 'scss'});
let sassAST = gonzales.parse(fs.readFileSync(sassFilePath).toString(), {syntax: 'sass'});

scssAST

with dos line endings

Node {
  type: 'stylesheet',
  ... 
  start: { line: 1, column: 1 },
  end: { line: 3, column: 3 } }

with unix line ending

Node {
  type: 'stylesheet',
  ...
  start: { line: 1, column: 1 },
  end: { line: 3, column: 2 } }

scssAST traverse to comment Node

scssAST.traverseByTypes(['multilineComment', 'singlelineComment'], function(comment, i, parent) {console.log(parent)}) logs with both unix or dos line endings

Node {
  type: 'block',
  ...
  start: { line: 1, column: 3 },
  end: { line: 3, column: 1 } }

sassAST

with dos line endings

Node {
  type: 'stylesheet',
  ...
  start: { line: 1, column: 1 },
  end: { line: 2, column: 34 } }

with unix line endings

Node {
  type: 'stylesheet',
  ...
  start: { line: 1, column: 1 },
  end: { line: 2, column: 33 } }

sassAST traverse to comment Node

sassAST.traverseByTypes(['multilineComment', 'singlelineComment'], function(comment, i, parent) {console.log(parent)}) logs with dos line endings

Node {
  type: 'block',
  ...
  start: { line: 2, column: 1 },
  end: { line: 2, column: 33 } }

logs with unix line endings

Node {
  type: 'block',
  ...
  start: { line: 2, column: 1 },
  end: { line: 2, column: 32 } }

hope this helps

@glen-84
Copy link
Contributor

glen-84 commented Jul 25, 2016

@DanPurdy Any news here? #677 will be really useful.

@DanPurdy
Copy link
Member

Hi @glen-84 I just haven't had any time recently i'm afraid. I will get round to this shortly though!

@DanPurdy
Copy link
Member

DanPurdy commented Aug 8, 2016

Right going to merge this into the disable-linters branch and look into the problem as soon as possible.

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.

5 participants