Repository: skovhus/jest-codemods
Pull request #291 (2022-06-08 22:26:51)

Make sinon.stub() mock implementation when nothing is returned (& other tweaks)
cc @lencioni @catc

In some recent PRs I noticed that `sinon.stub(a, b)` is transformed into `jest.spyOn(a, b).mockClear()` but it also needs a `.mockImplementation()` in the general case (when there's not a `.return()` or `.returnArg()`).  So I've added that.

Additionally, I read sinon's doc on `stub()` and `spy()` and we don't need/implement `stub(a)` (which mocks all keys in the object) so I'm marking that as unsupported (it's not advised even from sinon's doc).

I added a warning when there's more than 3 arguments (which doesn't seem to be valid, AFAICT).

I also changed `jest.fn().mockImplementation(...)` to just `jest.fn(...)` because it's equivalent and shorter.

codecov[bot]: # [Codecov](https://codecov.io/gh/skovhus/jest-codemods/pull/291?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kenneth+Skovhus) Report
> Merging [#291](https://codecov.io/gh/skovhus/jest-codemods/pull/291?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kenneth+Skovhus) (43f869a) into [main](https://codecov.io/gh/skovhus/jest-codemods/commit/93d99e022e479c0909e3c89938b51153d9a647ba?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kenneth+Skovhus) (93d99e0) will **increase** coverage by `0.04%`.
> The diff coverage is `100.00%`.

```diff
@@            Coverage Diff             @@
##             main     #291      +/-   ##
==========================================
+ Coverage   92.18%   92.22%   +0.04%     
==========================================
  Files          26       26              
  Lines        1842     1852      +10     
  Branches      381      385       +4     
==========================================
+ Hits         1698     1708      +10     
  Misses        102      102              
  Partials       42       42              
```


| [Impacted Files](https://codecov.io/gh/skovhus/jest-codemods/pull/291?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kenneth+Skovhus) | Coverage Δ | |
|---|---|---|
| [src/transformers/sinon.ts](https://codecov.io/gh/skovhus/jest-codemods/pull/291/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kenneth+Skovhus#diff-c3JjL3RyYW5zZm9ybWVycy9zaW5vbi50cw==) | `87.31% <100.00%> (+0.65%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/skovhus/jest-codemods/pull/291?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kenneth+Skovhus).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kenneth+Skovhus)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/skovhus/jest-codemods/pull/291?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kenneth+Skovhus). Last update [93d99e0...43f869a](https://codecov.io/gh/skovhus/jest-codemods/pull/291?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kenneth+Skovhus). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kenneth+Skovhus).

skovhus: This is awesome! Thanks for fixing this. 👏 

Patch:
diff --git a/src/transformers/sinon.ts b/src/transformers/sinon.ts
index 1e53c43..46b656f 100644
--- a/src/transformers/sinon.ts
+++ b/src/transformers/sinon.ts
@@ -8,6 +8,7 @@ import {
 } from '../utils/chai-chain-utils'
 import finale from '../utils/finale'
 import { removeDefaultImport } from '../utils/imports'
+import logger from '../utils/logger'
 import { findParentOfType } from '../utils/recast-helpers'
 import {
   expressionContainsProperty,
@@ -52,6 +53,7 @@ const SINON_MATCHERS_WITH_ARGS = {
 }
 const SINON_NTH_CALLS = new Set(['firstCall', 'secondCall', 'thirdCall', 'lastCall'])
 const EXPECT_PREFIXES = new Set(['to'])
+const isPrefix = (name) => EXPECT_PREFIXES.has(name)
 
 /* 
   expect(spy.called).to.be(true) -> expect(spy).toHaveBeenCalled()
@@ -102,7 +104,6 @@ function transformCallCountAssertions(j, ast) {
 
       const expectArgSinonMethod = expectArg.property.name
 
-      const isPrefix = (name) => EXPECT_PREFIXES.has(name)
       const negated =
         chainContains('not', node.callee, isPrefix) || node.arguments?.[0].value === false // eg: .to.be(false)
       const rest = getAllBefore(isPrefix, node.callee, 'should')
@@ -164,7 +165,6 @@ function transformCalledWithAssertions(j, ast) {
         })
 
       const expectArgSinonMethod = expectArg.callee?.property?.name
-      const isPrefix = (name) => EXPECT_PREFIXES.has(name)
       const negated =
         chainContains('not', node.callee, isPrefix) || node.arguments?.[0].value === false // eg: .to.be(false)
       const rest = getAllBefore(isPrefix, node.callee, 'should')
@@ -183,7 +183,7 @@ function transformCalledWithAssertions(j, ast) {
 /* 
 sinon.stub(Api, 'get') -> jest.spyOn(Api, 'get')
 */
-function transformStub(j: core.JSCodeshift, ast, sinonExpression) {
+function transformStub(j: core.JSCodeshift, ast, sinonExpression, logWarning) {
   ast
     .find(j.CallExpression, {
       callee: {
@@ -199,9 +199,18 @@ function transformStub(j: core.JSCodeshift, ast, sinonExpression) {
       },
     })
     .replaceWith((np) => {
+      // stubbing/spyOn module
       const args = np.value.arguments
+      const propertyName = np.node.callee.property.name
+
+      if (args.length === 1 && propertyName === 'stub') {
+        logWarning(
+          'stubbing all methods in an object is not supported; stub each one you care about individually',
+          np
+        )
+        return np.value
+      }
 
-      // stubbing/spyOn module
       if (args.length >= 2) {
         let spyOn = j.callExpression(
           j.memberExpression(j.identifier('jest'), j.identifier('spyOn')),
@@ -212,27 +221,49 @@ function transformStub(j: core.JSCodeshift, ast, sinonExpression) {
         spyOn = j.callExpression(j.memberExpression(spyOn, j.identifier('mockClear')), [])
 
         // add mockImplementation call
-        if (args.length === 3) {
+        if (args.length >= 3) {
           spyOn = j.callExpression(
             j.memberExpression(spyOn, j.identifier('mockImplementation')),
             [args[2]]
           )
+
+          if (args.length >= 4) {
+            logWarning(
+              `4+ arguments found in sinon.${propertyName} call; did you mean to use this many?`,
+              np
+            )
+          }
+        } else if (propertyName === 'stub') {
+          const parent =
+            findParentOfType(np, 'VariableDeclaration') ||
+            findParentOfType(np, 'ExpressionStatement')
+
+          const hasReturn =
+            j(parent)
+              .find(j.CallExpression, {
+                callee: {
+                  type: 'MemberExpression',
+                  property: {
+                    type: 'Identifier',
+                    name: (name) => ['returns', 'returnsArg'].includes(name),
+                  },
+                },
+              })
+              .size() > 0
+
+          if (!hasReturn) {
+            spyOn = j.callExpression(
+              j.memberExpression(spyOn, j.identifier('mockImplementation')),
+              []
+            )
+          }
         }
 
         return spyOn
       }
 
-      const jestFnCall = j.callExpression(j.identifier('jest.fn'), [])
-
-      if (args.length === 1) {
-        return j.callExpression(
-          j.memberExpression(jestFnCall, j.identifier('mockImplementation')),
-          args
-        )
-      }
-
       // jest mock function
-      return jestFnCall
+      return j.callExpression(j.identifier('jest.fn'), args)
     })
 }
 
@@ -650,7 +681,9 @@ export default function transformer(fileInfo: FileInfo, api: API, options) {
     return null
   }
 
-  transformStub(j, ast, sinonExpression)
+  const logWarning = (msg, node) => logger(fileInfo, msg, node)
+
+  transformStub(j, ast, sinonExpression, logWarning)
   transformMockTimers(j, ast)
   transformMock(j, ast)
   transformMockResets(j, ast)


Test patch:
diff --git a/src/transformers/sinon.test.ts b/src/transformers/sinon.test.ts
index 54d74c6..b987915 100644
--- a/src/transformers/sinon.test.ts
+++ b/src/transformers/sinon.test.ts
@@ -11,10 +11,13 @@ beforeEach(() => {
   jest.spyOn(console, 'warn').mockImplementation().mockClear()
 })
 
-function expectTransformation(source, expectedOutput, options = {}) {
-  const result = wrappedPlugin(source, options)
+function expectTransformation(source, expectedOutput, warnings = []) {
+  const result = wrappedPlugin(source)
   expect(result).toBe(expectedOutput)
-  expect(console.warn).toBeCalledTimes(0)
+  expect(console.warn).toBeCalledTimes(warnings.length)
+  warnings.forEach((warning, i) => {
+    expect(console.warn).nthCalledWith(i + 1, warning)
+  })
 }
 
 it('removes imports', () => {
@@ -34,21 +37,37 @@ describe('spies and stubs', () => {
     expectTransformation(
       `
         import sinon from 'sinon-sandbox'
-        const stub = sinon.stub(Api, 'get')
+
+        const stub = sinon.stub(Api, 'get');
+        const putOk = sinon.stub(Api, 'put').returns(200)
+        sinon.stub(I18n); // unsupported
         sinon.stub(I18n, 'extend');
-        sinon.stub(AirbnbUser, 'current').returns(currentUser);
+        sinon.stub(AirbnbUser, 'current').returnsArg(1);
+        sinon.stub(a, b, () => c, d) // too many args
+
         sinon.spy(I18n, 'extend');
         sinon.spy();
         sinon.spy(() => 'foo');
+        sinon.spy(a, b, () => c, d) // too many args
 `,
       `
-        const stub = jest.spyOn(Api, 'get').mockClear()
-        jest.spyOn(I18n, 'extend').mockClear();
-        jest.spyOn(AirbnbUser, 'current').mockClear().mockReturnValue(currentUser);
+        const stub = jest.spyOn(Api, 'get').mockClear().mockImplementation();
+        const putOk = jest.spyOn(Api, 'put').mockClear().mockReturnValue(200)
+        sinon.stub(I18n); // unsupported
+        jest.spyOn(I18n, 'extend').mockClear().mockImplementation();
+        jest.spyOn(AirbnbUser, 'current').mockClear().mockImplementation((...args) => args[1]);
+        jest.spyOn(a, b).mockClear().mockImplementation(() => c) // too many args
+
         jest.spyOn(I18n, 'extend').mockClear();
         jest.fn();
-        jest.fn().mockImplementation(() => 'foo');
-`
+        jest.fn(() => 'foo');
+        jest.spyOn(a, b).mockClear().mockImplementation(() => c) // too many args
+`,
+      [
+        'jest-codemods warning: (test.js line 6) stubbing all methods in an object is not supported; stub each one you care about individually',
+        'jest-codemods warning: (test.js line 9) 4+ arguments found in sinon.stub call; did you mean to use this many?',
+        'jest-codemods warning: (test.js line 14) 4+ arguments found in sinon.spy call; did you mean to use this many?',
+      ]
     )
   })
 
@@ -79,8 +98,8 @@ describe('spies and stubs', () => {
 `,
       `
         beforeEach(() => {
-          jest.spyOn(Api, 'get').mockClear()
-          const s1 = jest.spyOn(I18n, 'extend').mockClear()
+          jest.spyOn(Api, 'get').mockClear().mockImplementation()
+          const s1 = jest.spyOn(I18n, 'extend').mockClear().mockImplementation()
           const s2 = jest.spyOn(I18n, 'extend').mockClear().mockReturnValue('en')
           jest.spyOn(L10n, 'language').mockClear().mockReturnValue('en')
           jest.spyOn(I18n, 'extend').mockClear().mockImplementation(() => 'foo');