Skip to content

Commit 83db8f6

Browse files
codejedi365codejedi365
committed
fix: expectExpect rule to handle more use cases
\### Rationale Due to the lack of type-checking and not utilizing recursion to review the function tree, there were some missing cases where the rule would fail. Added more test cases which highlight code sequences that do fail on the old implementation. Also, made strict adjustments to jest threshold related to this file's error handling. So if this changes, it will have to be determined of a better implementation.
1 parent 3b3bdb7 commit 83db8f6

File tree

3 files changed

+252
-48
lines changed

3 files changed

+252
-48
lines changed

‎jest.config.js‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,10 @@ module.exports = {
88
functions: 100,
99
lines: 90,
1010
statements: 90
11+
},
12+
"./lib/rules/expect-expect.ts": { // allow error handling
13+
lines: -3,
14+
statements: -5
1115
}
1216
},
1317
testPathIgnorePatterns: ["<rootDir>/tests/fixtures/"],

‎lib/rules/expect-expect.ts‎

Lines changed: 161 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,108 @@
11
/**
2-
* @fileoverview Don't allow debug() to be committed to the repository.
2+
* @fileoverview Don't allow debug() to be committed to the repository.
33
* @author Ben Monro
4+
* @author codejedi365
45
*/
5-
"use strict";
6-
6+
import {
7+
CallExpression,
8+
MemberExpression,
9+
Identifier,
10+
AST_NODE_TYPES,
11+
BaseNode
12+
} from "@typescript-eslint/types/dist/ast-spec";
713
import { createRule } from "../create-rule";
814

15+
type FunctionName = string;
16+
type ObjectName = string;
17+
18+
const testFnAttributes = [
19+
// Derived List from TestFn class of [email protected]/ts-defs/index.d.ts
20+
// - only extracted attributes which return the testFn object (this)
21+
// which are possible modifiers to a test call before the test callback
22+
// is defined
23+
"only",
24+
"skip",
25+
"disablePageCaching",
26+
"disablePageReloads"
27+
];
28+
29+
function isMemberExpression(node: BaseNode): node is MemberExpression {
30+
return node.type === AST_NODE_TYPES.MemberExpression;
31+
}
32+
33+
function isIdentifier(node: BaseNode): node is Identifier {
34+
return node.type === AST_NODE_TYPES.Identifier;
35+
}
36+
37+
function isCallExpression(node: BaseNode): node is CallExpression {
38+
return node.type === AST_NODE_TYPES.CallExpression;
39+
}
40+
41+
function digForIdentifierName(startNode: BaseNode): string {
42+
function checkTypeForRecursion(
43+
node: BaseNode
44+
): node is CallExpression | MemberExpression | Identifier {
45+
return (
46+
isIdentifier(node) ||
47+
isMemberExpression(node) ||
48+
isCallExpression(node)
49+
);
50+
}
51+
function deriveFnName(
52+
node: CallExpression | MemberExpression | Identifier
53+
): string {
54+
let nextNode: BaseNode = node;
55+
56+
if (isCallExpression(node)) {
57+
nextNode = node.callee;
58+
} else if (isMemberExpression(node)) {
59+
nextNode = node.object;
60+
} else if (isIdentifier(node)) {
61+
return node.name;
62+
}
63+
64+
if (!checkTypeForRecursion(nextNode)) throw new Error();
65+
return deriveFnName(nextNode);
66+
}
67+
68+
// Start Point
69+
try {
70+
if (!checkTypeForRecursion(startNode)) throw new Error();
71+
return deriveFnName(startNode);
72+
} catch (e) {
73+
throw new Error("Could not derive function name from callee.");
74+
}
75+
}
76+
77+
function deriveFunctionName(fnCall: CallExpression): string {
78+
const startNode =
79+
isMemberExpression(fnCall.callee) &&
80+
isIdentifier(fnCall.callee.property)
81+
? fnCall.callee.property
82+
: fnCall.callee;
83+
84+
return digForIdentifierName(startNode);
85+
}
86+
87+
/**
88+
* Must detect symbol names in the following syntatical situations
89+
* 1. stand-alone function call (identifier only)
90+
* 2. object class method call (MemberExpression)
91+
* 3. n+ deep object attributes (Recursive MemberExpressions)
92+
* 4. when expression Is on a method chain (Recursive CallExpressions)
93+
* @param fnCall
94+
* @returns top level symbol for name of object
95+
*/
96+
function deriveObjectName(fnCall: CallExpression): string {
97+
return digForIdentifierName(fnCall.callee);
98+
}
99+
100+
function determineCodeLocation(
101+
node: CallExpression
102+
): [FunctionName, ObjectName] {
103+
return [deriveFunctionName(node), deriveObjectName(node)];
104+
}
105+
9106
//------------------------------------------------------------------------------
10107
// Rule Definition
11108
//------------------------------------------------------------------------------
@@ -14,9 +111,9 @@ export default createRule({
14111
name: __filename,
15112
defaultOptions: [],
16113
meta: {
17-
type:"problem",
114+
type: "problem",
18115
messages: {
19-
missingExpect: 'Please ensure your test has at least one expect'
116+
missingExpect: "Please ensure your test has at least one expect"
20117
},
21118
docs: {
22119
description: "Ensure tests have at least one expect",
@@ -25,35 +122,67 @@ export default createRule({
25122
},
26123
schema: []
27124
},
28-
create: function(context) {
29-
30-
let hasExpect = false;
31-
let isInsideTest = false;
125+
create(context) {
126+
let hasExpect = false;
127+
let isInsideTest = false;
128+
let ignoreExpects = false;
32129
return {
33-
"CallExpression"(node: any) {
34-
const name = node.callee.name || node.callee.property?.name;
35-
const objectName = node.callee.object?.name || node.callee.callee?.object?.object?.name || node.parent.callee?.callee?.object?.name;
36-
if (name === "test" || objectName === "test") {
37-
isInsideTest = true;
130+
"CallExpression": (node: CallExpression) => {
131+
if (isInsideTest && hasExpect) return; // Short circuit, already found
132+
133+
let fnName;
134+
let objectName;
135+
try {
136+
[fnName, objectName] = determineCodeLocation(node);
137+
} catch (e) {
138+
// ABORT: Failed to evaluate rule effectively
139+
// since I cannot derive values to determine location in the code
140+
return;
38141
}
39-
if (isInsideTest && name === "expect") {
40-
hasExpect = true;
142+
143+
if (isInsideTest) {
144+
if (ignoreExpects) return;
145+
if (fnName === "expect") {
146+
hasExpect = true;
147+
return;
148+
}
149+
if (objectName === "test") {
150+
// only happens in chained methods with internal callbacks
151+
// like test.before(() => {})("my test", async () => {})
152+
// prevents any registering of an expect in the before() callback
153+
ignoreExpects = true;
154+
}
155+
return;
41156
}
42-
},
43-
44-
"CallExpression:exit"(node: any) {
45-
const name = node.callee.name || node.callee.property?.name;
46-
47-
const objectName = node.callee.object?.name || node.callee.callee?.object?.object?.name || node.parent.callee?.callee.object.name;
48-
if (name === "test" || objectName === "test") {
49-
if (!hasExpect) {
50-
context.report({ node, messageId: "missingExpect" });
51-
}
52-
hasExpect = false;
53-
isInsideTest = false;
157+
// Determine if inside/chained to a test() function
158+
if (objectName !== "test") return;
159+
if (fnName === "test" || testFnAttributes.includes(fnName)) {
160+
isInsideTest = true;
54161
}
55-
}
56-
}
162+
},
163+
164+
"CallExpression:exit": (node: CallExpression) => {
165+
if (!isInsideTest) return; // Short circuit
166+
167+
let fnName;
168+
let objectName;
169+
try {
170+
[fnName, objectName] = determineCodeLocation(node);
171+
} catch (e) {
172+
// ABORT: Failed to evaluate rule effectively
173+
// since I cannot derive values to determine location in the code
174+
return;
175+
}
176+
if (objectName !== "test") return;
177+
if (fnName === "test" || testFnAttributes.includes(fnName)) {
178+
if (!hasExpect) {
179+
context.report({ node, messageId: "missingExpect" });
180+
}
181+
hasExpect = false;
182+
isInsideTest = false;
183+
}
184+
ignoreExpects = false;
185+
}
186+
};
57187
}
58-
}
59-
);
188+
});

‎tests/lib/rules/expect-expect.ts‎

Lines changed: 87 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1,54 +1,125 @@
11
/**
2-
* @fileoverview Don&#39;t allow debug() to be committed to the repository.
2+
* @fileoverview Don&#39;t allow debug() to be committed to the repository.
33
* @author Ben Monro
4+
* @author codejedi365
45
*/
5-
"use strict";
66

77
//------------------------------------------------------------------------------
88
// Requirements
99
//------------------------------------------------------------------------------
10+
import resolveFrom from "resolve-from";
11+
import { TSESLint } from "@typescript-eslint/experimental-utils";
1012
import rule from "../../../lib/rules/expect-expect";
11-
import {RuleTester} from 'eslint';
12-
13-
14-
import resolveFrom from 'resolve-from';
15-
import { TSESLint } from '@typescript-eslint/experimental-utils';
1613

1714
//------------------------------------------------------------------------------
1815
// Tests
1916
//------------------------------------------------------------------------------
20-
let ruleTester = new TSESLint.RuleTester({ parser: resolveFrom(require.resolve('eslint'), 'espree'),
21-
parserOptions: { ecmaVersion: 8 } });
17+
const ruleTester = new TSESLint.RuleTester({
18+
parser: resolveFrom(require.resolve("eslint"), "espree"),
19+
parserOptions: { ecmaVersion: 8 }
20+
});
2221

2322
ruleTester.run("expect-expect", rule, {
2423
valid: [
25-
`test("foo", async t => { await t.expect(foo).eql(bar)})`,
26-
`test.skip("foo", async t => { await t.expect(foo).eql(bar)})`
24+
`test("foo", async t => { await t.expect(foo).eql(bar) })`,
25+
`test.skip("foo", async t => { await t.expect(foo).eql(bar) })`,
26+
`test.page("./foo")("foo", async t => { await t.expect(foo).eql(bar) })`,
27+
`test.only.page("./foo")("foo", async t => { await t.expect(foo).eql(bar) })`,
28+
// Chained expect
29+
`test("foo", async t => {
30+
await t
31+
.click(button)
32+
.expect(foo)
33+
.eql(bar)
34+
})`,
35+
// More than 1 function on t
36+
`test("foo", async t => {
37+
await t.click(button)
38+
await t.expect(foo).eql(bar)
39+
})`,
40+
// Multiple expects
41+
`test("foo", async t => {
42+
await t.click(button)
43+
await t.expect(foo).eql(bar)
44+
await t.expect(true).ok()
45+
})`,
46+
// chained function with callback parameter
47+
`test.before(async t => {
48+
await t.useRole(adminRole).wait(1000);
49+
})("foo", async t => {
50+
await t.click(button);
51+
await t.expect(foo).eql(bar);
52+
})`,
53+
// Multiple tests
54+
`fixture("My Fixture")
55+
.page("https://example.com");
56+
57+
test("test1", async t => {
58+
await t.useRole(adminRole);
59+
await t.expect(foo).eql(bar);
60+
});
61+
test("test2", async t => {
62+
await t.click(button);
63+
await t.expect(foo).eql(bar);
64+
});`
2765
],
2866
invalid: [
2967
{
3068
code: `test("foo", async t => {
3169
await t.click(button)
3270
})`,
33-
errors:[{messageId: "missingExpect"}]
71+
errors: [{ messageId: "missingExpect" }]
3472
},
3573
{
3674
code: `test.skip("foo", async t => {
3775
await t.click(button)
3876
})`,
39-
errors:[{messageId: "missingExpect"}]
77+
errors: [{ messageId: "missingExpect" }]
4078
},
4179
{
4280
code: `test.page("./foo")("foo", async t => {
4381
await t.click(button)
4482
})`,
45-
errors:[{messageId: "missingExpect"}]
83+
errors: [{ messageId: "missingExpect" }]
4684
},
4785
{
4886
code: `test.skip.page("./foo")("foo", async t => {
4987
await t.click(button)
5088
})`,
51-
errors:[{messageId: "missingExpect"}]
89+
errors: [{ messageId: "missingExpect" }]
90+
},
91+
{
92+
code: `test.before(async t => {
93+
await t.useRole(adminRole).wait(1000);
94+
await t.expect(Login).ok();
95+
})("foo", async t => {
96+
await t.click(button);
97+
})`,
98+
errors: [{ messageId: "missingExpect" }]
99+
},
100+
{
101+
code: `test("foo", async t => {
102+
await t
103+
.useRole(adminRole)
104+
.click(button)
105+
.wait(1000)
106+
})`,
107+
errors: [{ messageId: "missingExpect" }]
108+
},
109+
{
110+
// Missing one expect across 2 tests
111+
code: `fixture("My Fixture")
112+
.page("https://example.com");
113+
114+
test("test1", async t => {
115+
await t.useRole(adminRole).wait(500);
116+
await t.expect(foo).eql(bar);
117+
});
118+
119+
test("test2", async t => {
120+
await t.click(button);
121+
});`,
122+
errors: [{ messageId: "missingExpect" }]
52123
}
53124
]
54-
})
125+
});

0 commit comments

Comments
 (0)