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

Fix tests for function-calc-no-invalid #4533

Closed
LolliDepp opened this issue Jan 9, 2020 · 8 comments
Closed

Fix tests for function-calc-no-invalid #4533

LolliDepp opened this issue Jan 9, 2020 · 8 comments
Labels
status: wip is being worked on by someone type: tests an improvement to testing

Comments

@LolliDepp
Copy link

Clearly describe the bug

With function-calc-no-invalid there is a false positive, while invalid syntax is allowed.

Which rule, if any, is the bug related to?

function-calc-no-invalid

What CSS is needed to reproduce the bug?

This is valid LESS and should be accepted.

// style.less
@myVar: 100px;

a {
  width: @myVar;
}

This is invalid LESS, but gets flagged as correct.

// style.less
@myVar: 100px;

a {
  width: @{myVar};
}

More than this, the second scenario is used as a positive test case for that rule!
So that's definitely broken.
Stick the LESS in here to verify: https://lesstester.com/

What stylelint configuration is needed to reproduce the bug?

{
  "rules": {
    "function-calc-no-invalid": "on"
  }
}

Which version of stylelint are you using?

12.0.1

How are you running stylelint: CLI, PostCSS plugin, Node.js API?

VSCode extension running it on the currently open file: stuartzhang.stylelint-stzhang

Does the bug relate to non-standard syntax (e.g. SCSS, Less etc.)?

Yes, it's related to LESS variables

What did you expect to happen?

No warnings to be flagged with the valid syntax, warnings flagged with invalid syntax.
I also expect the test cases on the project to not explicitly allow invalid syntax.

What actually happened (e.g. what warnings or errors did you get)?

Invalid syntax is not flagged, valid syntax is flagged.
Unit tests are wrong.

width: calc(@{min-value} + @{max-value}*(100vw - @{min-vw}));

That line that is in the list of syntax to parse as valid makes no sense at all, it's just not valid LESS.
@{my-var} is for interpolation in properties names, class name, and internal to string values.
@my-var is for using variables as values for properties.

width: calc(@{min-value} + @{max-value}*(100vw -@{min-vw}));

This line that is in the list of syntax to parse with errors should be parsed with errors, but not with the function-calc-no-invalid error. The error you're supposed to get is "Right curly bracket expected but not found". It just doesn't make any sense and should probably trigger a bunch of errors.

Working on a PR with fixed tests...

@LolliDepp LolliDepp changed the title Tests for function-calc-no-invalid allow invalid syntax, flags valid syntax Tests for function-calc-no-invalid allow invalid syntax, rule flags valid syntax and allows invalid Jan 9, 2020
@LolliDepp LolliDepp changed the title Tests for function-calc-no-invalid allow invalid syntax, rule flags valid syntax and allows invalid (LESS) Tests for function-calc-no-invalid allow invalid syntax, rule flags valid syntax and allows invalid Jan 9, 2020
@LolliDepp
Copy link
Author

LolliDepp commented Jan 9, 2020

PR with changes to tests to highlight the issues: #4534

@fanich37
Copy link
Member

Thank you for the report.
Stylelint doesn't check validity of CSS/SASS/LESS.

This line that is in the list of syntax to parse with errors should be parsed with errors, but not with the function-calc-no-invalid error.

Each rule is reporting exact violations that is described in the doc:
https://github.com/stylelint/stylelint/blob/d3c4a9f50bb77fd9014e04b84e9b336978239d97/lib/rules/function-calc-no-invalid/README.md

I think it's good idea to fix syntax in tests to valid.

@jeddy3
Copy link
Member

jeddy3 commented Jan 11, 2020

I think it's good idea to fix syntax in tests to valid.

Agreed.

@jeddy3 jeddy3 changed the title (LESS) Tests for function-calc-no-invalid allow invalid syntax, rule flags valid syntax and allows invalid Fix tests for function-calc-no-invalid Jan 12, 2020
@jeddy3 jeddy3 added status: wip is being worked on by someone type: tests an improvement to testing labels Jan 12, 2020
@jeddy3
Copy link
Member

jeddy3 commented Jan 12, 2020

@LolliDepp Thanks for the report.

OK, it took a little while to work through this so let's break it down:

This is invalid LESS, but gets flagged as correct.

As @fanich37 said, stylelint doesn't check the validity of non-standard syntax. If the postcss-less parser can parse it, stylelint will run its rules against it.

This is valid LESS and should be accepted. [...] valid syntax is flagged.

I can't replicate this.

% echo "@myVar: 100px; a { width: @myVar; }" | npx stylelint --syntax less --formatter verbose

1 source checked
 <input css 1>

0 problems found

Unit tests are wrong.

Yes, you're totally right. We should probably revisit all our Less tests to make sure we're not making similar mistakes elsewhere. I don't believe any of the core contributors use Less anymore so we rely on the help of outside contributors to support Less.

I've focused this issue on the incorrect syntax in the tests as I think the other parts of the issue are duplicated in #4114.

@glen-84
Copy link
Contributor

glen-84 commented Oct 31, 2021

@jeddy3 I guess this can be closed? (rule removed)

@jeddy3
Copy link
Member

jeddy3 commented Oct 31, 2021

@jeddy3 I guess this can be closed? (rule removed)

Yep. We need to go through all the open issues and close those that are no longer relevant now that 14.0.0 is out. I'll try to make time for it some weekend soon.

@jeddy3 jeddy3 closed this as completed Oct 31, 2021
@jeddy3
Copy link
Member

jeddy3 commented Oct 31, 2021

We need to go through all the open issues and close those that are no longer relevant now that 14.0.0 is out.

@glen-84 Feel free to close them yourself too. I think you have permission to do so as a contributor. We can always reopen them if they're closed by mistake.

@glen-84
Copy link
Contributor

glen-84 commented Nov 1, 2021

@jeddy3 I don't have permission. I just noticed these two while reading through the changelog.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: wip is being worked on by someone type: tests an improvement to testing
Development

No branches or pull requests

4 participants