Fix tests that use callbacks #4881

ybiquitous · 2020-07-26T14:05:09Z

Clearly describe the bug

I found a bug in the following test code using a callback:

stylelint/lib/utils/__tests__/isStandardSyntaxRule.test.js

Lines 58 to 62 in 9f93d1f

    
           it('called Less class parametric mixin', () => { 
        
           	lessRules('.mixin-name(@var);', (rule) => { 
        
           		expect(isStandardSyntaxRule(rule)).toBeFalsy(); 
        
           	}); 
        
           });

When I add done() to the test as follows, this test fails.

--- a/lib/utils/__tests__/isStandardSyntaxRule.test.js
+++ b/lib/utils/__tests__/isStandardSyntaxRule.test.js
@@ -55,9 +55,10 @@ describe('isStandardSyntaxRule', () => {
 			expect(isStandardSyntaxRule(rule)).toBeFalsy();
 		});
 	});
-	it('called Less class parametric mixin', () => {
+	it('called Less class parametric mixin', (done) => {
 		lessRules('.mixin-name(@var);', (rule) => {
 			expect(isStandardSyntaxRule(rule)).toBeFalsy();
+			done();
 		});
 	});
 	it('non-outputting parametric Less class mixin definition', () => {

$ npx jest --silent lib/utils/__tests__/isStandardSyntaxRule.test.js
 FAIL  lib/utils/__tests__/isStandardSyntaxRule.test.js (5.515 s)
  ● isStandardSyntaxRule › called Less class parametric mixin

    thrown: "Exceeded timeout of 5000 ms for a test.
    Use jest.setTimeout(newTimeout) to increase the timeout value, if this is a long-running test."

      56 | 		});
      57 | 	});
    > 58 | 	it('called Less class parametric mixin', (done) => {
         | 	^
      59 | 		lessRules('.mixin-name(@var);', (rule) => {
      60 | 			expect(isStandardSyntaxRule(rule)).toBeFalsy();
      61 | 			done();

      at lib/utils/__tests__/isStandardSyntaxRule.test.js:58:2
      at Object.<anonymous> (lib/utils/__tests__/isStandardSyntaxRule.test.js:7:1)

Test Suites: 1 failed, 1 total
Tests:       1 failed, 30 passed, 31 total
Snapshots:   0 total
Time:        5.544 s, estimated 6 s

Because this Less node .mixin-name(@var); is an AtRule (not Rule), and thus expect() is not called in the callback passed to lessRules().

When I rewrite the test without a callback as follows, the test fails still, though.
However, the failure is due to passing an AtRule to isStandardSyntaxRule(), and I believe there is no such a case actually.
(Perhaps, we can fix the failure by returning false if rule.type !== 'rule'.)

--- a/lib/utils/__tests__/isStandardSyntaxRule.test.js
+++ b/lib/utils/__tests__/isStandardSyntaxRule.test.js
@@ -56,9 +56,8 @@ describe('isStandardSyntaxRule', () => {
 		});
 	});
 	it('called Less class parametric mixin', () => {
-		lessRules('.mixin-name(@var);', (rule) => {
-			expect(isStandardSyntaxRule(rule)).toBeFalsy();
-		});
+		const rule = less.parse('.mixin-name(@var);').first;
+		expect(isStandardSyntaxRule(rule)).toBeFalsy();
 	});
 	it('non-outputting parametric Less class mixin definition', () => {
 		lessRules('.mixin-name() {}', (rule) => {

$ npx jest --silent lib/utils/__tests__/isStandardSyntaxRule.test.js
 FAIL  lib/utils/__tests__/isStandardSyntaxRule.test.js
  ● isStandardSyntaxRule › called Less class parametric mixin

    TypeError: Cannot read property 'startsWith' of undefined

      16 |
      17 | 	// SCSS placeholder selectors
    > 18 | 	if (selector.startsWith('%')) {
         | 	             ^
      19 | 		return false;
      20 | 	}
      21 |

      at isStandardSyntaxSelector (lib/utils/isStandardSyntaxSelector.js:18:15)
      at isStandardSyntaxRule (lib/utils/isStandardSyntaxRule.js:17:7)
      at Object.<anonymous> (lib/utils/__tests__/isStandardSyntaxRule.test.js:60:10)

Test Suites: 1 failed, 1 total
Tests:       1 failed, 30 passed, 31 total
Snapshots:   0 total
Time:        0.567 s, estimated 1 s

There are such invalid test codes using a callback in the other test files, e.g.:

stylelint/lib/utils/__tests__/isStandardSyntaxAtRule.test.js

Lines 10 to 14 in 9f93d1f

    
           it('non nested at-rules without quotes', () => { 
        
           	atRules('@charset UTF-8;', (atRule) => { 
        
           		expect(isStandardSyntaxAtRule(atRule)).toBeTruthy(); 
        
           	}); 
        
           });

Sadly, the ESLint jest/no-test-callback rule cannot detect such problematic code patterns... 😢
(see https://github.com/jest-community/eslint-plugin-jest/blob/v23.18.2/docs/rules/no-test-callback.md)

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

None.

What code is needed to reproduce the bug?

See the description above about the reproduction.

What stylelint configuration is needed to reproduce the bug?

None.

Which version of stylelint are you using?

The HEAD version.

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

Via Jest, e.g. npx jest lib/utils/__tests__/isStandardSyntaxRule.test.js

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

No.

What did you expect to happen?

I think we should not use a callback in the test code as possible.
Or, if a callback is needed, we should wrap the callback with return new Promise(done => {})

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

Some tests do not work correctly.

The text was updated successfully, but these errors were encountered:

jeddy3 · 2020-08-01T18:12:31Z

I think we should not use a callback in the test code as possible.
Or, if a callback is needed, we should wrap the callback with return new Promise(done => {})

SGTM.

ybiquitous · 2020-08-22T15:38:52Z

@jeddy3 Thanks for the feedback!

There are many tests to be fixed, so I think it better to tackle this issue by multiple pull requests to help reviewers.

This is a part of #4881.

ybiquitous · 2021-04-06T16:37:56Z

I find 2 issues discussing similar problems to this issue:

Unfortunately, it seems there is not an easy way. 😓

ybiquitous · 2021-04-09T15:22:20Z

FYI. If this ESLint rule (maybe jest/prefer-expect-resolves) would be implemented, this task might be easier.

ybiquitous · 2021-04-09T15:28:42Z

G-Rath · 2021-04-10T22:15:35Z

I wonder if it could be valuable for eslint-plugin-jest to ship something like no-expects-in-callback that could take an allowlist to provide a hyper-aggressive way of detecting this 🤔

Unfortunately, it seems there is not an easy way. 😓

@ybiquitous you should be able to create a file like this that calls expect.hasAssertions before every test.

That won't work if you have tests that don't use expect, but depending on what those tests are (and how many there are) there are ways around that - i.e converting test logic into a custom matcher on expect, or adding "fake" (or less useful) expect calls to those tests to ensure you've got at least one expect.

I'll try to see if I can get onto shipping prefer-expect-resolves this week for you :)

ybiquitous · 2021-04-11T03:18:29Z

@ybiquitous you should be able to create a file like this that calls expect.hasAssertions before every test.

@G-Rath Thank you for the advice! That’s also a nice idea (I didn't know such a way). 😄

But, I’m a bit concerned about the way that at least one expect() may be required would make some developers confusing, since it would introduce an explicit context (maybe as you also pointed). 🤔

So, I think it best if we can check them via ESLint.
Again, thank you so much for your feedback! 🙌

- Remove the `del` package that will be needless. - Use asnyc/await syntax for readability. (#4881) - Fix CSS code indent for consistency.

- Remove the `del` package that will be needless. - Use async/await syntax for readability. (#4881) - Fix CSS code indent for consistency.

- Reduce redundant callbacks via `async/await` syntax. (#4881) - Use `fs.existsSync()` instead of `fs.access()`. See <https://nodejs.org/api/fs.html#fs_fs_existssync_path> - Fix the disabled test. (#5309) - Inline redundant local variables. - Make expectations using `typeof` more accurate.

* Refactor `lib/__tests__/standalone-cache.test.js` - Reduce redundant callbacks via `async/await` syntax. (#4881) - Use `fs.existsSync()` instead of `fs.access()`. See <https://nodejs.org/api/fs.html#fs_fs_existssync_path> - Fix the disabled test. (#5309) - Inline redundant local variables. - Make expectations using `typeof` more accurate. * Reduce local variables by improving `getConfig()`

ybiquitous added the type: tests an improvement to testing label Jul 26, 2020

jeddy3 changed the title ~~Broken tests using callback~~ Fix tests that use callbacks Aug 1, 2020

jeddy3 added the status: ready to implement is ready to be worked on by someone label Aug 1, 2020

ybiquitous mentioned this issue Aug 22, 2020

Fix isStandardSyntaxRule.test.js that use callbacks #4902

Merged

ybiquitous mentioned this issue Aug 22, 2020

Fix callbacks in tests #4903

Merged

This was referenced Sep 2, 2020

Fix isLessVariable.test.js that use callbacks #4921

Merged

Fix isKeyframeRule.test.js that use callbacks #4922

Merged

ybiquitous added a commit that referenced this issue Sep 9, 2020

Fix isKeyframeRule.test.js that use callbacks (#4922)

ea9b7ca

This is a part of #4881.

ybiquitous added a commit that referenced this issue Sep 9, 2020

Fix isLessVariable.test.js that use callbacks (#4921)

97eb96d

This is a part of #4881.

This was referenced Sep 25, 2020

Fix isStandardSyntaxAtRule.test.js that use callbacks #4957

Merged

Fix isSharedLineComment.test.js that uses callbacks #4958

Merged

Fix isStandardSyntaxCombinator.test.js that use callbacks #4959

Merged

This was referenced Oct 7, 2020

Fix some tests that use callbacks #4970

Merged

Fix isStandardSyntaxDeclaration.test.js that use callbacks #4972

Merged

This was referenced Oct 16, 2020

Fix isStandardSyntaxTypeSelector.test.js that use callbacks #4990

Merged

Fix disableRanges.test.js that uses callbacks #4991

Merged

This was referenced Apr 6, 2021

Refactor lib/__tests__/postcssPlugin.test.js #5231

Merged

Refactor lib/__tests__/createLinter.test.js #5232

Merged

Refactor lib/__tests__/defaultSeverity.test.js #5233

Merged

Refactor lib/__tests__/descriptionlessDisables.test.js #5234

Merged

ybiquitous mentioned this issue Apr 9, 2021

Refactor lib/utils/__tests__/nodeContextLookup.test.js #5238

Merged

ybiquitous added a commit that referenced this issue Apr 30, 2021

Refactor lib/__tests__/standalone-fix.test.js

0c3a7d1

- Remove the `del` package that will be needless. - Use asnyc/await syntax for readability. (#4881) - Fix CSS code indent for consistency.

ybiquitous added a commit that referenced this issue Apr 30, 2021

Refactor lib/__tests__/standalone-fix.test.js

70d7486

- Remove the `del` package that will be needless. - Use async/await syntax for readability. (#4881) - Fix CSS code indent for consistency.

ybiquitous mentioned this issue Apr 30, 2021

Refactor lib/__tests__/standalone-fix.test.js #5270

Merged

jeddy3 pushed a commit that referenced this issue Apr 30, 2021

Refactor lib/__tests__/standalone-fix.test.js

a9d476b

- Remove the `del` package that will be needless. - Use async/await syntax for readability. (#4881) - Fix CSS code indent for consistency.

jeddy3 pushed a commit that referenced this issue Apr 30, 2021

Refactor lib/__tests__/standalone-fix.test.js (#5270)

c52f9ae

- Remove the `del` package that will be needless. - Use async/await syntax for readability. (#4881) - Fix CSS code indent for consistency.

ybiquitous mentioned this issue May 17, 2021

Refactor lib/__tests__/standalone-cache.test.js #5311

Merged

ybiquitous mentioned this issue Jun 14, 2022

Refactor to remove callbacks in test examples #6149

Merged

ybiquitous closed this as completed in #6149 Jun 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix tests that use callbacks #4881

Fix tests that use callbacks #4881

ybiquitous commented Jul 26, 2020

jeddy3 commented Aug 1, 2020

ybiquitous commented Aug 22, 2020

ybiquitous commented Apr 6, 2021

ybiquitous commented Apr 9, 2021

ybiquitous commented Apr 9, 2021 •

edited

G-Rath commented Apr 10, 2021 •

edited

ybiquitous commented Apr 11, 2021

Fix tests that use callbacks #4881

Fix tests that use callbacks #4881

Comments

ybiquitous commented Jul 26, 2020

jeddy3 commented Aug 1, 2020

ybiquitous commented Aug 22, 2020

ybiquitous commented Apr 6, 2021

ybiquitous commented Apr 9, 2021

ybiquitous commented Apr 9, 2021 • edited

G-Rath commented Apr 10, 2021 • edited

ybiquitous commented Apr 11, 2021

ybiquitous commented Apr 9, 2021 •

edited

G-Rath commented Apr 10, 2021 •

edited