Skip to content

Commit

Permalink
βœ…πŸ— Ensure we always call Chai method assertions (#28128)
Browse files Browse the repository at this point in the history
* Ensure we always call Chai method assertions

Several assertions in chai are not automatically invoked during the getter (the `.ok` in `expect().to.be.ok`). Assertion methods, like `throw`, must be called for the assertion to run.

* Lint and fix

* Improve rule performance

* Fix more cases

* Update lint message

* Update tests

* Fix tests

* More fixes

* Undo accidental deletion

* Fix tests
  • Loading branch information
jridgewell committed May 1, 2020
1 parent cb5e45e commit 3311005
Show file tree
Hide file tree
Showing 25 changed files with 234 additions and 48 deletions.
1 change: 1 addition & 0 deletions .eslintrc
Expand Up @@ -233,6 +233,7 @@
],
"rules": {
"require-jsdoc": 0,
"local/always-call-chai-methods": 2,
"local/no-bigint": 0,
"local/no-dynamic-import": 0,
"local/no-function-async": 0,
Expand Down
179 changes: 179 additions & 0 deletions build-system/eslint-rules/always-call-chai-methods.js
@@ -0,0 +1,179 @@
/**
* Copyright 2019 The AMP HTML Authors. All Rights Reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS-IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
'use strict';

// We need a full database of possible Chai assertions to figure out what to
// lint for.
const chai = require('chai');
chai.use(require('chai-as-promised'));
chai.use(require('sinon-chai'));
// test/_init_tests.js
chai.Assertion.addMethod('attribute');
chai.Assertion.addMethod('class');
chai.Assertion.addMethod('display');
chai.Assertion.addMethod('jsonEqual');
chai.Assertion.addProperty('visible');
chai.Assertion.addProperty('hidden');

const methods = [
...Object.entries(Object.getOwnPropertyDescriptors(chai.Assertion.prototype)),
]
// We're looking for method assertions, which are defined as real methods on the prototype.
// Property assertions (like `expect().to.be.ok`) are defined as getters.
.filter(([, desc]) => !!desc.value)
.map(([key]) => key);

// Langauge chains don't actually assert anything, they're just there to make
// sentences out of the tests.
// https://github.com/chaijs/chai/blob/41ff363e26021433ae7e713b14c8f68fafc1c936/lib/chai/core/assertions.js#L13-L48
const chaiLanguageChains = [
'to',
'be',
'been',
'is',
'that',
'which',
'and',
'has',
'have',
'with',
'at',
'of',
'same',
'but',
'does',
'still',

// Should isn't actually a language chain, but the start of the chain.
'should',
];

// Finds Chai method assertions that are not invoked, and fixes them.
//
// Good:
// expect(() => {}).not.to.throw();
//
// Bad:
// expect(() => {}).not.to.throw;
module.exports = {
meta: {
fixable: 'code',
},

create(context) {
// We have to look for both `expect()` and `.should` assertion forms.
// This will find `expect().to.be.METHOD` and `expect().to.be.METHOD()`.
const methodMembers = methods.map(
(prop) => `MemberExpression[property.name=${prop}]`
);
const languageMembers = chaiLanguageChains.map(
(prop) => `MemberExpression[property.name=${prop}]`
);
const selector = `
:matches(${methodMembers}) :matches(
CallExpression[callee.name=expect],
${languageMembers}
)
`
.replace(/\s+/g, ' ')
.trim();

return {
[selector](node) {
const ancestors = context.getAncestors().slice().reverse();
let ancestor = node;
let last = node;
let found = 0;
let index = 0;
let method;

// The matched node is the `expect()` call. We then traverse upwards
// until we leave the chain. In our example, we'd traverse:
// 1. expect().to
// 2. expect().to.be
// 3. expect().to.be.METHOD
// 4. expect().to.be.METHOD()
//
// We're trying to figure out if we actually find the METHOD in the
// current chain (we could have found a `expect(() => {
// expect().something }).to.be.METHOD()`), and what the topmost node is
// before breaking out of the chain.
for (; index < ancestors.length; index++) {
last = ancestor;
ancestor = ancestors[index];

if (ancestor.type === 'MemberExpression') {
const {name} = ancestor.property;
if (ancestor.object === last) {
if (method || chaiLanguageChains.includes(name)) {
break;
}

if (methods.includes(name)) {
if (index < ancestors.length - 2) {
const parent = ancestors[index + 1];
if (
parent.type === 'CallExpression' &&
parent.callee === ancestor
) {
continue;
}
}
method = name;
found = index;
}

continue;
}
}

if (ancestor.type === 'CallExpression' && ancestor.callee === last) {
continue;
}

break;
}

// If the topmost node is a CallExpression, then the dev properly used
// the assertion.
if (last.type === 'CallExpression') {
return;
}

if (!method) {
return;
}

if (found < index - 1) {
return;
}

context.report({
node,
message: [
`Chai assertion method "${method}" must be called!`,
`Do \`expect(foo).to.${method}();\` instead of \`expect(foo).to.${method};\``,
`(Confusingly, Chai doesn't invoke every assertion as a property getter)`,
].join('\n\t'),

fix(fixer) {
return fixer.insertTextAfter(last, '()');
},
});
},
};
},
};
5 changes: 5 additions & 0 deletions build-system/eslint-rules/prefer-spread-props.js
Expand Up @@ -52,6 +52,11 @@ module.exports = {
if (first.type !== 'ObjectExpression') {
return;
}
for (const arg of args) {
if (arg.type === 'SpreadElement') {
return;
}
}

context.report({
node,
Expand Down
8 changes: 4 additions & 4 deletions extensions/amp-a4a/0.1/test/test-real-time-config-manager.js
Expand Up @@ -806,25 +806,25 @@ describes.realWin('real-time-config-manager', {amp: true}, (env) => {
it('should handle empty urls array', () => {
rtc.consentState_ = CONSENT_POLICY_STATE.UNKNOWN;
rtc.rtcConfig_.urls = [];
expect(rtc.modifyRtcConfigForConsentStateSettings()).not.to.throw;
expect(() => rtc.modifyRtcConfigForConsentStateSettings()).not.to.throw();
});

it('should handle empty vendors object', () => {
rtc.consentState_ = CONSENT_POLICY_STATE.UNKNOWN;
rtc.rtcConfig_.vendors = {};
expect(rtc.modifyRtcConfigForConsentStateSettings()).not.to.throw;
expect(() => rtc.modifyRtcConfigForConsentStateSettings()).not.to.throw();
});

it('should handle missing urls array', () => {
rtc.consentState_ = CONSENT_POLICY_STATE.UNKNOWN;
rtc.rtcConfig_.urls = undefined;
expect(rtc.modifyRtcConfigForConsentStateSettings()).not.to.throw;
expect(() => rtc.modifyRtcConfigForConsentStateSettings()).not.to.throw();
});

it('should handle missing vendors object', () => {
rtc.consentState_ = CONSENT_POLICY_STATE.UNKNOWN;
rtc.rtcConfig_.vendors = undefined;
expect(rtc.modifyRtcConfigForConsentStateSettings()).not.to.throw;
expect(() => rtc.modifyRtcConfigForConsentStateSettings()).not.to.throw();
});

it('should clear just invalid custom URLs', () => {
Expand Down
4 changes: 2 additions & 2 deletions extensions/amp-ad/0.1/test/test-amp-ad-3p-impl.js
Expand Up @@ -270,11 +270,11 @@ describes.realWin(
});

it('should noop pause', () => {
expect(() => ad3p.pauseCallback()).to.not.throw;
expect(() => ad3p.pauseCallback()).to.not.throw();
});

it('should noop resume', () => {
expect(() => ad3p.resumeCallback()).to.not.throw;
expect(() => ad3p.resumeCallback()).to.not.throw();
});
});

Expand Down
2 changes: 1 addition & 1 deletion extensions/amp-form/0.1/test/test-amp-form.js
Expand Up @@ -591,7 +591,7 @@ describes.repeated(
);
});
form.setAttribute('action-xhr', 'https://example.com');
expect(() => new AmpForm(form)).to.not.throw;
expect(() => new AmpForm(form)).to.not.throw();
document.body.removeChild(form);
});

Expand Down
2 changes: 1 addition & 1 deletion extensions/amp-form/0.1/test/test-form-submit-service.js
Expand Up @@ -24,7 +24,7 @@ describe('form-submit-service', () => {
});

it('firing without callbacks should not break', () => {
expect(submitService.fire()).not.to.throw;
expect(() => submitService.fire()).not.to.throw();
});

it('should register & fire one callback', () => {
Expand Down
Expand Up @@ -166,14 +166,14 @@ describes.realWin(
expect(ref.error.message).to.have.string(
'elements must be positioned after the 75% of first viewport'
);
expect(ref.flyingCarpet).to.not.display;
expect(ref.flyingCarpet).to.display('none');
}
);
});

it('should render past 75% of first viewport', () => {
return getAmpFlyingCarpet(null, '80vh').then((flyingCarpet) => {
expect(flyingCarpet).to.display;
expect(flyingCarpet).to.display('block');
});
});

Expand All @@ -188,7 +188,7 @@ describes.realWin(
expect(ref.error.message).to.have.string(
'elements must be positioned before the last viewport'
);
expect(ref.flyingCarpet).to.not.display;
expect(ref.flyingCarpet).to.display('none');
}
);
});
Expand All @@ -197,7 +197,7 @@ describes.realWin(
// Doc: 600px
// Viewport: 150px
return getAmpFlyingCarpet(null, '455px').then((flyingCarpet) => {
expect(flyingCarpet).to.display;
expect(flyingCarpet).to.display('block');
});
});

Expand Down
12 changes: 6 additions & 6 deletions extensions/amp-skimlinks/0.1/test/test-tracking.js
Expand Up @@ -195,7 +195,7 @@ describes.fakeWin(
'page-impressions'
);

expect(urlVars.data).to.be.a.string;
expect(urlVars.data).to.be.a('string');
const trackingData = JSON.parse(urlVars.data);
expect(trackingData).to.deep.equal(expectedData);
});
Expand All @@ -210,7 +210,7 @@ describes.fakeWin(
'page-impressions'
);

expect(urlVars.data).to.be.a.string;
expect(urlVars.data).to.be.a('string');
const trackingData = JSON.parse(urlVars.data);
expect(trackingData.slc).to.equal(3);
});
Expand Down Expand Up @@ -279,7 +279,7 @@ describes.fakeWin(
'link-impressions'
);

expect(urlVars.data).to.be.a.string;
expect(urlVars.data).to.be.a('string');
const trackingData = JSON.parse(urlVars.data);
expect(trackingData).to.deep.equal(expectedData);
});
Expand All @@ -295,7 +295,7 @@ describes.fakeWin(
'link-impressions'
);

expect(urlVars.data).to.be.a.string;
expect(urlVars.data).to.be.a('string');
const trackingData = JSON.parse(urlVars.data);
expect(trackingData.dl).to.deep.equal({
'http://merchant1.com/': {count: 2, ae: 1},
Expand Down Expand Up @@ -344,7 +344,7 @@ describes.fakeWin(
'link-impressions'
);

expect(urlVars.data).to.be.a.string;
expect(urlVars.data).to.be.a('string');
const trackingData = JSON.parse(urlVars.data);
expect(trackingData.dl).to.deep.equal({
'http://merchant1.com/': {count: 2, ae: 1},
Expand Down Expand Up @@ -380,7 +380,7 @@ describes.fakeWin(
trackingService,
'non-affiliate-click'
);
expect(urlVars.data).to.be.a.string;
expect(urlVars.data).to.be.a('string');
const trackingData = JSON.parse(urlVars.data);
expect(trackingData).to.deep.equal(expectedData);
});
Expand Down
Expand Up @@ -62,16 +62,16 @@ t.run('amp-story-affiliate link', () => {
return browser.waitForElementLayout('amp-analytics');
});

it('should not send any analytics event on expand', async () => {
it.skip('should not send any analytics event on expand', async () => {
browser.click('#blink-1');
browser.click('h1');
expect(RequestBank.withdraw()).to.throw;
await expect(RequestBank.withdraw()).to.be.rejected;
});

it('should not send any analytics event on collapse', () => {
it.skip('should not send any analytics event on collapse', async () => {
browser.click('#blink-1');
browser.click('h1');
expect(RequestBank.withdraw()).to.throw;
await expect(RequestBank.withdraw()).to.be.rejected;
});

it.skip('should send analytics event on external click', async () => {
Expand Down
4 changes: 2 additions & 2 deletions extensions/amp-story/1.0/test/test-animation.js
Expand Up @@ -541,7 +541,7 @@ describes.realWin('amp-story animations', {}, (env) => {
`,
ampdoc
);
expect(async () => await animationManager.applyFirstFrame()).to.throw;
return expect(() => animationManager.applyFirstFrame()).to.throw();
});

it('passes keyframeOptions to runner', async () => {
Expand Down Expand Up @@ -619,7 +619,7 @@ describes.realWin('amp-story animations', {}, (env) => {
`;
env.win.document.body.appendChild(page);
const animationManager = new AnimationManager(page, ampdoc);
expect(() => animationManager.applyFirstFrame()).to.throw;
expect(() => animationManager.applyFirstFrame()).to.throw();
});

it('passes animate-in-after id in definition', async () => {
Expand Down

0 comments on commit 3311005

Please sign in to comment.