From a02e4cea94279ea5e350fde1b56a30b8cd1895da Mon Sep 17 00:00:00 2001 From: Luke Page Date: Tue, 25 Dec 2018 20:00:36 +0100 Subject: [PATCH 1/4] Add new rule - assertion before screenshot --- README.md | 1 + docs/rules/assertion-before-screenshot.md | 22 +++++ index.js | 1 + lib/rules/assertion-before-screenshot.js | 97 +++++++++++++++++++ package-lock.json | 22 +++-- .../lib/rules/assertion-before-screenshot.js | 34 +++++++ 6 files changed, 171 insertions(+), 6 deletions(-) create mode 100644 docs/rules/assertion-before-screenshot.md create mode 100644 lib/rules/assertion-before-screenshot.js create mode 100644 tests/lib/rules/assertion-before-screenshot.js diff --git a/README.md b/README.md index ddd2e717..7429bc3d 100644 --- a/README.md +++ b/README.md @@ -49,6 +49,7 @@ Rules with a check mark (✅) are enabled by default while using the `plugin:cyp |:---|:--------|:------------| | ✅ | [no-assigning-return-values](./docs/rules/no-assigning-return-values.md) | Prevent assigning return values of cy calls | | ✅ | [no-unnecessary-waiting](./docs/rules/no-unnecessary-waiting.md) | Prevent waiting for arbitrary time periods | +| | [assertion-before-screenshot](./docs/rules/assertion-before-screenshot.md) | Ensure screenshots are preceded by an assertion | ## Chai and `no-unused-expressions` diff --git a/docs/rules/assertion-before-screenshot.md b/docs/rules/assertion-before-screenshot.md new file mode 100644 index 00000000..d144690c --- /dev/null +++ b/docs/rules/assertion-before-screenshot.md @@ -0,0 +1,22 @@ +## Assertion Before Screenshot + +If you take screenshots without assertions then you may get different screenshots depending on timing. + +For example, if clicking a button makes some network calls and upon success, renders something, then the screenshot may sometimes have the new render and sometimes not. + +This rule checks there is an assertion making sure your application state is correct before doing a screenshot. This makes sure the result of the screenshot will be consistent. + +Invalid: + +``` +cy.visit('myUrl'); +cy.screenshot(); +``` + +Valid: + +``` +cy.visit('myUrl'); +cy.get('[data-test-id="my-element"]').should('be.visible'); +cy.screenshot(); +``` diff --git a/index.js b/index.js index 84788c5c..289ab72b 100644 --- a/index.js +++ b/index.js @@ -4,6 +4,7 @@ module.exports = { rules: { 'no-assigning-return-values': require('./lib/rules/no-assigning-return-values'), 'no-unnecessary-waiting': require('./lib/rules/no-unnecessary-waiting'), + 'assertion-before-screenshot': require('./lib/rules/assertion-before-screenshot'), }, configs: { recommended: require('./lib/config/recommended'), diff --git a/lib/rules/assertion-before-screenshot.js b/lib/rules/assertion-before-screenshot.js new file mode 100644 index 00000000..8b260b68 --- /dev/null +++ b/lib/rules/assertion-before-screenshot.js @@ -0,0 +1,97 @@ +/** + * @fileoverview Assert on the page state before taking a screenshot, so the screenshot is consistent + * @author Luke Page + */ + +'use strict' + +module.exports = { + meta: { + docs: { + description: 'Assert on the page state before taking a screenshot, so the screenshot is consistent', + category: 'Possible Errors', + recommended: true, + }, + schema: [], + messages: { + unexpected: 'Make an assertion on the page state before taking a screenshot', + }, + }, + create (context) { + return { + CallExpression (node) { + if (isCallingCyScreenshot(node) && !isPreviousAnAssertion(node)) { + context.report({ node, messageId: 'unexpected' }) + } + }, + } + }, +} + +function isRootCypress(node) { + while(node.type === 'CallExpression') { + if (node.callee.type === 'MemberExpression' && + node.callee.object.type === 'Identifier' && + node.callee.object.name === 'cy') { + return true + } + node = node.callee.object + } + return false +} + +function getPreviousInChain(node) { + return node.type === 'CallExpression' && + node.callee.type === 'MemberExpression' && + node.callee.object.type === 'CallExpression' && + node.callee.object.callee.type === 'MemberExpression' && + node.callee.object.callee.property.type === 'Identifier' && + node.callee.object.callee.property.name +} + +function getCallExpressionCypressCommand(node) { + return isRootCypress(node) && + node.callee.property.type === 'Identifier' && + node.callee.property.name +} + +function isCallingCyScreenshot (node) { + return getCallExpressionCypressCommand(node) === 'screenshot' +} + +function getPreviousCypressCommand(node) { + const previousInChain = getPreviousInChain(node) + + if (previousInChain) { + return previousInChain + } + + while(node.parent && !node.parent.body) { + node = node.parent + } + + if (!node.parent || !node.parent.body) return null + + const body = node.parent.body.type === 'BlockStatement' ? node.parent.body.body : node.parent.body + + const index = body.indexOf(node) + + // in the case of a function declaration it won't be found + if (index < 0) return null + + if (index === 0) return getPreviousCypressCommand(node.parent); + + const previousStatement = body[index - 1] + + if (previousStatement.type !== 'ExpressionStatement' || + previousStatement.expression.type !== 'CallExpression') + return null + + return getCallExpressionCypressCommand(previousStatement.expression) +} + +function isPreviousAnAssertion (node) { + const previousCypressCommand = getPreviousCypressCommand(node) + return previousCypressCommand === 'get' || + previousCypressCommand === 'should' +} diff --git a/package-lock.json b/package-lock.json index 1cf1c9bb..047e5345 100644 --- a/package-lock.json +++ b/package-lock.json @@ -6557,12 +6557,14 @@ "balanced-match": { "version": "1.0.0", "bundled": true, - "dev": true + "dev": true, + "optional": true }, "brace-expansion": { "version": "1.1.11", "bundled": true, "dev": true, + "optional": true, "requires": { "balanced-match": "^1.0.0", "concat-map": "0.0.1" @@ -6577,17 +6579,20 @@ "code-point-at": { "version": "1.1.0", "bundled": true, - "dev": true + "dev": true, + "optional": true }, "concat-map": { "version": "0.0.1", "bundled": true, - "dev": true + "dev": true, + "optional": true }, "console-control-strings": { "version": "1.1.0", "bundled": true, - "dev": true + "dev": true, + "optional": true }, "core-util-is": { "version": "1.0.2", @@ -6704,7 +6709,8 @@ "inherits": { "version": "2.0.3", "bundled": true, - "dev": true + "dev": true, + "optional": true }, "ini": { "version": "1.3.5", @@ -6716,6 +6722,7 @@ "version": "1.0.0", "bundled": true, "dev": true, + "optional": true, "requires": { "number-is-nan": "^1.0.0" } @@ -6730,6 +6737,7 @@ "version": "3.0.4", "bundled": true, "dev": true, + "optional": true, "requires": { "brace-expansion": "^1.1.7" } @@ -6841,7 +6849,8 @@ "number-is-nan": { "version": "1.0.1", "bundled": true, - "dev": true + "dev": true, + "optional": true }, "object-assign": { "version": "4.1.1", @@ -6974,6 +6983,7 @@ "version": "1.0.2", "bundled": true, "dev": true, + "optional": true, "requires": { "code-point-at": "^1.0.0", "is-fullwidth-code-point": "^1.0.0", diff --git a/tests/lib/rules/assertion-before-screenshot.js b/tests/lib/rules/assertion-before-screenshot.js new file mode 100644 index 00000000..3e48d0f3 --- /dev/null +++ b/tests/lib/rules/assertion-before-screenshot.js @@ -0,0 +1,34 @@ +'use strict' + +const rule = require('../../../lib/rules/assertion-before-screenshot') +const RuleTester = require('eslint').RuleTester + +const ruleTester = new RuleTester() + +const errors = [{ messageId: 'unexpected' }] +const parserOptions = { ecmaVersion: 6 } + +ruleTester.run('assertion-before-screenshot', rule, { + valid: [ + { code: 'cy.get(".some-element"); cy.screenshot();', parserOptions }, + { code: 'cy.get(".some-element").screenshot();', parserOptions }, + { code: 'cy.get(".some-element").should("be.visible"); cy.screenshot();', parserOptions }, + { code: 'cy.get(".some-element").screenshot().click()', parserOptions, errors }, + { code: 'cy.get(".some-element"); if(true) cy.screenshot();', parserOptions }, + { code: 'if(true) { cy.get(".some-element"); cy.screenshot(); }', parserOptions }, + { code: 'cy.get(".some-element"); if(true) { cy.screenshot(); }', parserOptions }, + { code: 'const a = () => { cy.get(".some-element"); cy.screenshot(); }', parserOptions, errors }, + ], + + invalid: [ + { code: 'cy.screenshot()', parserOptions, errors }, + { code: 'cy.visit("somepage"); cy.screenshot();', parserOptions, errors }, + { code: 'cy.custom(); cy.screenshot()', parserOptions, errors }, + { code: 'cy.get(".some-element").click(); cy.screenshot()', parserOptions, errors }, + { code: 'cy.get(".some-element").click().screenshot()', parserOptions, errors }, + { code: 'if(true) { cy.get(".some-element").click(); cy.screenshot(); }', parserOptions, errors }, + { code: 'cy.get(".some-element").click(); if(true) { cy.screenshot(); }', parserOptions, errors }, + { code: 'cy.get(".some-element"); function a() { cy.screenshot(); }', parserOptions, errors }, + { code: 'cy.get(".some-element"); const a = () => { cy.screenshot(); }', parserOptions, errors }, + ], +}) From c8d3d73c6e77f5c02ad15862e3ab39433dfdc235 Mon Sep 17 00:00:00 2001 From: Luke Page Date: Wed, 26 Dec 2018 13:32:37 +0100 Subject: [PATCH 2/4] Fixes and refinements --- lib/rules/assertion-before-screenshot.js | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/lib/rules/assertion-before-screenshot.js b/lib/rules/assertion-before-screenshot.js index 8b260b68..20938f38 100644 --- a/lib/rules/assertion-before-screenshot.js +++ b/lib/rules/assertion-before-screenshot.js @@ -5,6 +5,17 @@ 'use strict' +const assertionCommands = [ + // assertions + 'should', + 'and', + 'contains', + + // not an assertion, but unlikely to require waiting for render + 'scrollIntoView', + 'scrollTo', +]; + module.exports = { meta: { docs: { @@ -30,11 +41,11 @@ module.exports = { function isRootCypress(node) { while(node.type === 'CallExpression') { - if (node.callee.type === 'MemberExpression' && - node.callee.object.type === 'Identifier' && + if (node.callee.type !== 'MemberExpression') return false + if (node.callee.object.type === 'Identifier' && node.callee.object.name === 'cy') { - return true - } + return true + } node = node.callee.object } return false @@ -92,6 +103,5 @@ function getPreviousCypressCommand(node) { function isPreviousAnAssertion (node) { const previousCypressCommand = getPreviousCypressCommand(node) - return previousCypressCommand === 'get' || - previousCypressCommand === 'should' + return assertionCommands.indexOf(previousCypressCommand) >= 0 } From 97d75ff0e660805e7a232f5b553f7511f1700b38 Mon Sep 17 00:00:00 2001 From: Luke Page Date: Wed, 26 Dec 2018 13:43:07 +0100 Subject: [PATCH 3/4] Final fixes and test fixes --- lib/rules/assertion-before-screenshot.js | 3 +++ tests/lib/rules/assertion-before-screenshot.js | 15 ++++++++------- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/lib/rules/assertion-before-screenshot.js b/lib/rules/assertion-before-screenshot.js index 20938f38..9195a17f 100644 --- a/lib/rules/assertion-before-screenshot.js +++ b/lib/rules/assertion-before-screenshot.js @@ -11,6 +11,9 @@ const assertionCommands = [ 'and', 'contains', + // retries until it gets something + 'get', + // not an assertion, but unlikely to require waiting for render 'scrollIntoView', 'scrollTo', diff --git a/tests/lib/rules/assertion-before-screenshot.js b/tests/lib/rules/assertion-before-screenshot.js index 3e48d0f3..1532badd 100644 --- a/tests/lib/rules/assertion-before-screenshot.js +++ b/tests/lib/rules/assertion-before-screenshot.js @@ -11,13 +11,14 @@ const parserOptions = { ecmaVersion: 6 } ruleTester.run('assertion-before-screenshot', rule, { valid: [ { code: 'cy.get(".some-element"); cy.screenshot();', parserOptions }, - { code: 'cy.get(".some-element").screenshot();', parserOptions }, - { code: 'cy.get(".some-element").should("be.visible"); cy.screenshot();', parserOptions }, - { code: 'cy.get(".some-element").screenshot().click()', parserOptions, errors }, - { code: 'cy.get(".some-element"); if(true) cy.screenshot();', parserOptions }, - { code: 'if(true) { cy.get(".some-element"); cy.screenshot(); }', parserOptions }, - { code: 'cy.get(".some-element"); if(true) { cy.screenshot(); }', parserOptions }, - { code: 'const a = () => { cy.get(".some-element"); cy.screenshot(); }', parserOptions, errors }, + { code: 'cy.get(".some-element").should("exist").screenshot();', parserOptions }, + { code: 'cy.get(".some-element").should("exist").screenshot().click()', parserOptions, errors }, + { code: 'cy.get(".some-element").should("exist"); if(true) cy.screenshot();', parserOptions }, + { code: 'if(true) { cy.get(".some-element").should("exist"); cy.screenshot(); }', parserOptions }, + { code: 'cy.get(".some-element").should("exist"); if(true) { cy.screenshot(); }', parserOptions }, + { code: 'const a = () => { cy.get(".some-element").should("exist"); cy.screenshot(); }', parserOptions, errors }, + { code: 'cy.get(".some-element").should("exist").and("be.visible"); cy.screenshot();', parserOptions }, + { code: 'cy.get(".some-element").contains("Text"); cy.screenshot();', parserOptions }, ], invalid: [ From 0b1b4b6116c6c1c7eaaeef5fd285e5c68ac1b700 Mon Sep 17 00:00:00 2001 From: Luke Page Date: Thu, 27 Dec 2018 19:42:08 +0100 Subject: [PATCH 4/4] Rule meta data - recommended off --- lib/rules/assertion-before-screenshot.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/rules/assertion-before-screenshot.js b/lib/rules/assertion-before-screenshot.js index 9195a17f..54688e01 100644 --- a/lib/rules/assertion-before-screenshot.js +++ b/lib/rules/assertion-before-screenshot.js @@ -24,7 +24,7 @@ module.exports = { docs: { description: 'Assert on the page state before taking a screenshot, so the screenshot is consistent', category: 'Possible Errors', - recommended: true, + recommended: false, }, schema: [], messages: {