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
GH-4917 Amend always test case #5050
GH-4917 Amend always test case #5050
Conversation
code: '// foo\n// bar\n@media {}', | ||
}, | ||
{ | ||
code: '// foo\n// bar\n\n@media{}', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m expecting this one to fail, given the failing parameters described in GH-4917.
@agarzola Thanks for starting to look into this.
You'll want to move those test cases into a new testRule({
ruleName,
config: ['always', { ignore: ['after-comment'] }],
syntax: "scss",
fix: true,
accept: [
{
code: '// foo\n// bar\n@media {}',
},
{
code: '// foo\n// bar\n\n@media{}',
},
{
code: '// foo\r\n// bar\r\n\r\n@media {}',
description: 'CRLF',
},
]
}); It'll then use the Scss parser rather than CSS one. You may be wondering why the CSS parser didn't error... well, double slashes are sometimes valid CSS, and can be parsed by the CSS parser. |
@jeddy3 Thank you for the speedy and very informative response! I had no idea |
5a05bfd
to
45d29c8
Compare
testRule({ | ||
ruleName, | ||
config: ['always', { ignore: ['after-comment'] }], | ||
syntax: 'scss', | ||
fix: true, | ||
|
||
accept: [ | ||
{ | ||
code: '// foo\n// bar\n@media {}', | ||
}, | ||
{ | ||
code: '// foo\n// bar\n\n@media{}', | ||
}, | ||
{ | ||
code: '// foo\r\n// bar\r\n\r\n@media {}', | ||
description: 'CRLF', | ||
}, | ||
], | ||
}); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jeddy3 I’ve separated out the test cases as you suggested, but it still passes. Any ideas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Never mind. I’m closing this PR, as the issue seems to have been resolved.
Closing this PR as it is no longer necessary. See #4917 (comment) for details. |
Closes GH-4917.
At the moment, yes. This is just a Draft PR because after amending the test cases for the
at-rule-empty-line-before
, tests pass. I have confirmed that the test cases I added are being executed. I would expect tests to show a failure, since the test cases I’m adding are representative of the bug described in GH-4917.I’ll appreciate any guidance on this issue, as I may be doing something wrong in my test cases.
Testing Instructions
For running tests locally, I’ve taken the following steps:
npm ci
npm run watch
o
to run files changed since last commit (before committing, of course).If you clone this branch and want to run this suite, you can:
npm run watch
p
option.at-rule-empty-line-before
.Enter
.You should see 1 test suite run with 362 tests passing and none failing.