Skip to content

Commit adbf0df

Browse files
bmeurerDevtools-frontend LUCI CQ
bmeurer
authored and
Devtools-frontend LUCI CQ
committed
[eslint] Prefer sinon assertions for better error messages.
The common pattern `assert(spy.called)` (and the like) yields pretty useless error messages (basically just stating that `true` was expected but `false` was found), which makes it a bit annoying to debug these failing tests. This introduces an ESLint rule that enforces the use of the `sinon.assert` API for the common cases, as these helpers yield much more actionable error messages. Bug: 397260638 Change-Id: I779c593048f4f91867b335072887d1ac2e4d81f9 Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/6450756 Commit-Queue: Nikolay Vitkov <nvitkov@chromium.org> Commit-Queue: Benedikt Meurer <bmeurer@chromium.org> Reviewed-by: Nikolay Vitkov <nvitkov@chromium.org> Auto-Submit: Benedikt Meurer <bmeurer@chromium.org>
1 parent f473909 commit adbf0df

File tree

120 files changed

+945
-629
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

120 files changed

+945
-629
lines changed

‎eslint.config.mjs

Lines changed: 13 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -199,7 +199,7 @@ export default [
199199
radix: 'error',
200200
'valid-typeof': 'error',
201201
'no-return-assign': ['error', 'always'],
202-
'no-implicit-coercion': ['error', { allow: ['!!'] }],
202+
'no-implicit-coercion': ['error', {allow: ['!!']}],
203203

204204
'no-array-constructor': 'error',
205205

@@ -310,11 +310,11 @@ export default [
310310
parserOptions: {
311311
allowAutomaticSingleRunInference: true,
312312
project: join(
313-
import.meta.dirname,
314-
'config',
315-
'typescript',
316-
'tsconfig.eslint.json',
317-
),
313+
import.meta.dirname,
314+
'config',
315+
'typescript',
316+
'tsconfig.eslint.json',
317+
),
318318
},
319319
},
320320

@@ -554,12 +554,12 @@ export default [
554554
{
555555
// Enforce that any import of models/trace/trace.js names the import Trace.
556556
modulePath: join(
557-
import.meta.dirname,
558-
'front_end',
559-
'models',
560-
'trace',
561-
'trace.js',
562-
),
557+
import.meta.dirname,
558+
'front_end',
559+
'models',
560+
'trace',
561+
'trace.js',
562+
),
563563
importName: 'Trace',
564564
},
565565
],
@@ -699,6 +699,7 @@ export default [
699699
'rulesdir/prefer-assert-instance-of': 'error',
700700
'rulesdir/prefer-assert-is-ok': 'error',
701701
'rulesdir/prefer-assert-length-of': 'error',
702+
'rulesdir/prefer-sinon-assert': 'error',
702703
'rulesdir/prefer-url-string': 'error',
703704
'rulesdir/trace-engine-test-timeouts': 'error',
704705
'rulesdir/enforce-custom-element-definitions-location': 'off',

‎front_end/core/common/Lazy.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,6 @@ describe('lazy', () => {
2323
// Subsequent calls of the function should throw an exception without
2424
// re-evaluation
2525
assert.throws(once, Error);
26-
assert.strictEqual(fake.callCount, 1);
26+
sinon.assert.callCount(fake, 1);
2727
});
2828
});

‎front_end/core/common/Settings.test.ts

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -271,7 +271,7 @@ describe('VersionController', () => {
271271
versionController.updateVersion();
272272

273273
for (const spy of spies) {
274-
assert.isFalse(spy.called);
274+
sinon.assert.notCalled(spy);
275275
}
276276
});
277277

@@ -286,7 +286,7 @@ describe('VersionController', () => {
286286
versionController.updateVersion();
287287

288288
for (const spy of spies) {
289-
assert.isFalse(spy.called);
289+
sinon.assert.notCalled(spy);
290290
}
291291
});
292292

@@ -303,12 +303,12 @@ describe('VersionController', () => {
303303

304304
const expectedUncalledSpies = spies.slice(0, -3);
305305
for (const spy of expectedUncalledSpies) {
306-
assert.isFalse(spy.called);
306+
sinon.assert.notCalled(spy);
307307
}
308308

309309
const expectedCalledSpies = spies.slice(-3);
310310
for (const spy of expectedCalledSpies) {
311-
assert.isTrue(spy.called);
311+
sinon.assert.called(spy);
312312
}
313313
});
314314

@@ -325,12 +325,12 @@ describe('VersionController', () => {
325325

326326
const expectedUncalledSpies = spies.slice(0, -1);
327327
for (const spy of expectedUncalledSpies) {
328-
assert.isFalse(spy.called);
328+
sinon.assert.notCalled(spy);
329329
}
330330

331331
const expectedCalledSpies = spies.slice(-1);
332332
for (const spy of expectedCalledSpies) {
333-
assert.isTrue(spy.called);
333+
sinon.assert.called(spy);
334334
}
335335
});
336336
});
@@ -641,13 +641,13 @@ describe('access logging', () => {
641641

642642
it('logs access on the first read', async () => {
643643
const setting = settings.createSetting('test-setting', false);
644-
assert.isFalse(logSettingAccess.called);
644+
sinon.assert.notCalled(logSettingAccess);
645645

646646
setting.get();
647647
assert.isTrue(logSettingAccess.calledOnceWith('test-setting', false));
648648

649649
setting.get();
650-
assert.isTrue(logSettingAccess.calledOnce);
650+
sinon.assert.calledOnce(logSettingAccess);
651651
});
652652

653653
it('logs access on the every write', async () => {
@@ -657,7 +657,7 @@ describe('access logging', () => {
657657
assert.isTrue(logSettingAccess.calledOnceWith('test-setting', true));
658658

659659
setting.set(false);
660-
assert.isTrue(logSettingAccess.calledTwice);
660+
sinon.assert.calledTwice(logSettingAccess);
661661
assert.deepEqual(logSettingAccess.secondCall.args, ['test-setting', false]);
662662
});
663663
});

‎front_end/core/common/Throttler.test.ts

Lines changed: 30 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -43,9 +43,9 @@ describe('Throttler class', () => {
4343
await ensureHasRecentRun();
4444
void throttler.schedule(process);
4545

46-
assert.isFalse(process.called);
46+
sinon.assert.notCalled(process);
4747
await clock.tickAsync(TIMEOUT + 1);
48-
assert.isTrue(process.calledOnce);
48+
sinon.assert.calledOnce(process);
4949
});
5050

5151
it('is able to schedule a process from process', async () => {
@@ -58,14 +58,14 @@ describe('Throttler class', () => {
5858
await ensureHasRecentRun();
5959
void throttler.schedule(process1);
6060

61-
assert.isFalse(process1.called);
61+
sinon.assert.notCalled(process1);
6262
assert.isFalse(process2.calledOnce);
6363
await clock.tickAsync(TIMEOUT + 1);
64-
assert.isTrue(process1.calledOnce);
64+
sinon.assert.calledOnce(process1);
6565
assert.isFalse(process2.calledOnce);
6666
await clock.tickAsync(TIMEOUT + 1);
67-
assert.isTrue(process1.calledOnce);
68-
assert.isTrue(process2.calledOnce);
67+
sinon.assert.calledOnce(process1);
68+
sinon.assert.calledOnce(process2);
6969
});
7070

7171
it('is able to schedule a process as soon as possible', async () => {
@@ -75,9 +75,9 @@ describe('Throttler class', () => {
7575
await ensureHasRecentRun();
7676
void throttler.schedule(process, Scheduling.AS_SOON_AS_POSSIBLE);
7777

78-
assert.isFalse(process.called);
78+
sinon.assert.notCalled(process);
7979
await clock.tickAsync(0);
80-
assert.isTrue(process.calledOnce);
80+
sinon.assert.calledOnce(process);
8181
});
8282

8383
it('is able to schedule two processes as soon as possible', async () => {
@@ -89,12 +89,12 @@ describe('Throttler class', () => {
8989
const promiseTest = throttler.schedule(process1, Scheduling.AS_SOON_AS_POSSIBLE);
9090
void throttler.schedule(process2, Scheduling.AS_SOON_AS_POSSIBLE);
9191

92-
assert.isFalse(process1.called);
93-
assert.isFalse(process2.called);
92+
sinon.assert.notCalled(process1);
93+
sinon.assert.notCalled(process2);
9494
void clock.tickAsync(0);
9595
await promiseTest;
96-
assert.isFalse(process1.called);
97-
assert.isTrue(process2.calledOnce);
96+
sinon.assert.notCalled(process1);
97+
sinon.assert.calledOnce(process2);
9898
});
9999

100100
it('by default schedules a process delayed only if another process ran recently', async () => {
@@ -103,26 +103,26 @@ describe('Throttler class', () => {
103103
throttler = new Throttler(TIMEOUT);
104104
void throttler.schedule(process);
105105

106-
assert.isFalse(process.called);
106+
sinon.assert.notCalled(process);
107107
await clock.tickAsync(0);
108-
assert.isTrue(process.calledOnce);
108+
sinon.assert.calledOnce(process);
109109
process.resetHistory();
110110

111111
void throttler.schedule(process);
112112
await clock.tickAsync(0);
113-
assert.isFalse(process.called);
113+
sinon.assert.notCalled(process);
114114
await clock.tickAsync(TIMEOUT / 2);
115-
assert.isFalse(process.called);
115+
sinon.assert.notCalled(process);
116116
void throttler.schedule(process);
117117
await clock.tickAsync(TIMEOUT / 2);
118-
assert.isTrue(process.calledOnce);
118+
sinon.assert.calledOnce(process);
119119

120120
await ensureNoRecentRun();
121121
process.resetHistory();
122122
void throttler.schedule(process);
123-
assert.isFalse(process.called);
123+
sinon.assert.notCalled(process);
124124
await clock.tickAsync(0);
125-
assert.isTrue(process.calledOnce);
125+
sinon.assert.calledOnce(process);
126126
});
127127

128128
it('is able to schedule a delayed process', async () => {
@@ -131,11 +131,11 @@ describe('Throttler class', () => {
131131
const throttler = new Throttler(TIMEOUT);
132132
void throttler.schedule(process, Scheduling.DELAYED);
133133

134-
assert.isFalse(process.called);
134+
sinon.assert.notCalled(process);
135135
await clock.tickAsync(0);
136-
assert.isFalse(process.called);
136+
sinon.assert.notCalled(process);
137137
await clock.tickAsync(TIMEOUT);
138-
assert.isTrue(process.calledOnce);
138+
sinon.assert.calledOnce(process);
139139
});
140140

141141
it('runs only one process at a time', async () => {
@@ -157,18 +157,18 @@ describe('Throttler class', () => {
157157
void throttler.schedule(process1, Scheduling.AS_SOON_AS_POSSIBLE);
158158

159159
await clock.tickAsync(0);
160-
assert.isTrue(spy1.called);
160+
sinon.assert.called(spy1);
161161

162162
void throttler.schedule(process2);
163163
await clock.tickAsync(100);
164-
assert.isFalse(spy2.called);
164+
sinon.assert.notCalled(spy2);
165165

166166
process1Resolve();
167167
await clock.tickAsync(0);
168-
assert.isFalse(spy2.called);
168+
sinon.assert.notCalled(spy2);
169169

170170
await clock.tickAsync(50);
171-
assert.isTrue(spy2.called);
171+
sinon.assert.called(spy2);
172172

173173
process2Resolve(); // No pending promises.
174174
});
@@ -179,12 +179,12 @@ describe('Throttler class', () => {
179179
const throttler = new Throttler(TIMEOUT);
180180
const promise = throttler.schedule(process, Scheduling.DELAYED);
181181

182-
assert.isTrue(process.notCalled);
182+
sinon.assert.notCalled(process);
183183
await clock.tickAsync(TIMEOUT + 1);
184-
assert.isTrue(process.calledOnce);
184+
sinon.assert.calledOnce(process);
185185
const race = await Promise.race([promise, Promise.resolve('pending')]);
186186
assert.notStrictEqual(race, 'pending');
187-
assert.isTrue(consoleStub.calledOnce);
187+
sinon.assert.calledOnce(consoleStub);
188188
});
189189

190190
it('is resolve promise correctly', async () => {
@@ -222,6 +222,6 @@ describe('Throttler class', () => {
222222
await clock.tickAsync(TIMEOUT + 1);
223223
assert.isFalse(process1.calledOnce);
224224
assert.isFalse(process2.calledOnce);
225-
assert.isTrue(process3.calledOnce);
225+
sinon.assert.calledOnce(process3);
226226
});
227227
});

‎front_end/core/sdk/AutofillModel.test.ts

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -19,30 +19,30 @@ describeWithMockConnection('AutofillModel', () => {
1919
const autofillModel = target.model(SDK.AutofillModel.AutofillModel);
2020
const enableSpy = sinon.spy(autofillModel!.agent, 'invoke_enable');
2121
const disableSpy = sinon.spy(autofillModel!.agent, 'invoke_disable');
22-
assert.isTrue(enableSpy.notCalled);
23-
assert.isTrue(disableSpy.notCalled);
22+
sinon.assert.notCalled(enableSpy);
23+
sinon.assert.notCalled(disableSpy);
2424

2525
autofillModel!.disable();
26-
assert.isTrue(enableSpy.notCalled);
27-
assert.isTrue(disableSpy.calledOnce);
26+
sinon.assert.notCalled(enableSpy);
27+
sinon.assert.calledOnce(disableSpy);
2828
disableSpy.resetHistory();
2929

3030
autofillModel!.enable();
31-
assert.isTrue(enableSpy.calledOnce);
32-
assert.isTrue(disableSpy.notCalled);
31+
sinon.assert.calledOnce(enableSpy);
32+
sinon.assert.notCalled(disableSpy);
3333
});
3434

3535
it('sets test addresses by calling the Autofill backend', () => {
3636
const target = createTarget();
3737
const autofillModel = target.model(SDK.AutofillModel.AutofillModel);
3838
const setAddressSpy = sinon.spy(autofillModel!.agent, 'invoke_setAddresses');
39-
assert.isTrue(setAddressSpy.notCalled);
39+
sinon.assert.notCalled(setAddressSpy);
4040

4141
autofillModel!.disable();
42-
assert.isTrue(setAddressSpy.notCalled);
42+
sinon.assert.notCalled(setAddressSpy);
4343

4444
autofillModel!.enable();
45-
assert.isTrue(setAddressSpy.calledOnce);
45+
sinon.assert.calledOnce(setAddressSpy);
4646
});
4747

4848
it('dispatches addressFormFilledEvent on autofill event', () => {

‎front_end/core/sdk/CPUThrottlingManager.test.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ describeWithMockConnection('CPUThrottlingManager', () => {
3232
const manager = SDK.CPUThrottlingManager.CPUThrottlingManager.instance();
3333
manager.setHardwareConcurrency(5);
3434

35-
assert.isTrue(cdpStub.calledOnce);
35+
sinon.assert.calledOnce(cdpStub);
3636
assert.isTrue(cdpStub.calledWithExactly({hardwareConcurrency: 5}));
3737
});
3838

@@ -41,9 +41,9 @@ describeWithMockConnection('CPUThrottlingManager', () => {
4141

4242
const manager = SDK.CPUThrottlingManager.CPUThrottlingManager.instance();
4343
manager.setHardwareConcurrency(0);
44-
assert.isFalse(cdpStub.called);
44+
sinon.assert.notCalled(cdpStub);
4545

4646
manager.setHardwareConcurrency(-1);
47-
assert.isFalse(cdpStub.called);
47+
sinon.assert.notCalled(cdpStub);
4848
});
4949
});

‎front_end/core/sdk/ChildTargetManager.test.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -210,14 +210,14 @@ describeWithMockConnection('ChildTargetManager', () => {
210210
SDK.ChildTargetManager.ChildTargetManager.install(attachCallback);
211211
await childTargetManager.attachedToTarget(
212212
{sessionId: createSessionId(), targetInfo: createTargetInfo(TARGET_ID), waitingForDebugger: false});
213-
assert.isTrue(attachCallback.calledOnce);
213+
sinon.assert.calledOnce(attachCallback);
214214
assert.strictEqual(attachCallback.firstCall.firstArg.target.id(), TARGET_ID);
215215
assert.isFalse(attachCallback.firstCall.firstArg.waitingForDebugger);
216216

217217
const OTHER_TARGET_ID = 'OTHER_TARGET_ID' as Protocol.Target.TargetID;
218218
await childTargetManager.attachedToTarget(
219219
{sessionId: createSessionId(), targetInfo: createTargetInfo(OTHER_TARGET_ID), waitingForDebugger: true});
220-
assert.isTrue(attachCallback.calledTwice);
220+
sinon.assert.calledTwice(attachCallback);
221221
assert.strictEqual(attachCallback.secondCall.firstArg.target.id(), OTHER_TARGET_ID);
222222
assert.isTrue(attachCallback.secondCall.firstArg.waitingForDebugger);
223223
});

0 commit comments

Comments
 (0)