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

GH-4917 Amend always test case #5050

Conversation

agarzola
Copy link

@agarzola agarzola commented Nov 20, 2020

Which issue, if any, is this issue related to?

Closes GH-4917.

Is there anything in the PR that needs further explanation?

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
  • Select option o to run files changed since last commit (before committing, of course).
  • After tests were done, I searched the console output to confirm that my test cases had indeed been run as part of the suite. Each one of them showed as passing.

If you clone this branch and want to run this suite, you can:

  • npm run watch
  • Select the p option.
  • Type at-rule-empty-line-before.
  • Use the down arrow key to select the only file that matches.
  • Press Enter.

You should see 1 test suite run with 362 tests passing and none failing.

code: '// foo\n// bar\n@media {}',
},
{
code: '// foo\n// bar\n\n@media{}',
Copy link
Author

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.

@jeddy3
Copy link
Member

jeddy3 commented Nov 20, 2020

@agarzola Thanks for starting to look into this.

I’ll appreciate any guidance on this issue, as I may be doing something wrong in my test cases.

You'll want to move those test cases into a new testRule that sets the syntax to "scss", like so:

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.

@agarzola
Copy link
Author

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 // comments could sometimes be valid CSS. Thanks again!

@agarzola agarzola force-pushed the gh-4917-fix-empty-line-before-w-double-slashed-comments branch from 5a05bfd to 45d29c8 Compare November 20, 2020 19:09
Comment on lines +300 to +319
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',
},
],
});

Copy link
Author

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?

Copy link
Author

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.

@agarzola
Copy link
Author

Closing this PR as it is no longer necessary. See #4917 (comment) for details.

@agarzola agarzola closed this Nov 20, 2020
@agarzola agarzola deleted the gh-4917-fix-empty-line-before-w-double-slashed-comments branch November 20, 2020 19:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Fix false positives for two double slashed comments in *-empty-line-before
2 participants