fix #1541: Improve deprecation messages (#1548)

afontcu · web-flow · commit c94c58985e9e · 2020-05-17T10:25:56.000+10:00
* docs: improve title on deprecation warnings

* docs: add suggestion for isVueInstance

* refactor: improve migration path for setMethods and options.methods

* docs: fix link format

* docs: add more hints to replace setMethods and options.methods

* docs: reword messages

* docs: remove implicit statement
diff --git a/docs/api/options.md b/docs/api/options.md
@@ -322,7 +322,7 @@ wrapper.destroy()
 - type: `boolean`
 - default: `false`
 
-::: warning
+::: warning Deprecation warning
 `attachToDocument` is deprecated and will be removed in future releases. Use [`attachTo`](#attachto) instead.
 :::
 
diff --git a/docs/api/wrapper-array/isEmpty.md b/docs/api/wrapper-array/isEmpty.md
@@ -1,6 +1,6 @@
 ## isEmpty
 
-::: warning
+::: warning Deprecation warning
 `isEmpty` is deprecated and will be removed in future releases.
 
 Consider a custom matcher such as those provided in [jest-dom](https://github.com/testing-library/jest-dom#tobeempty).
diff --git a/docs/api/wrapper-array/isVueInstance.md b/docs/api/wrapper-array/isVueInstance.md
@@ -1,7 +1,11 @@
 ## isVueInstance
 
-::: warning
+::: warning Deprecation warning
 `isVueInstance` is deprecated and will be removed in future releases.
+
+Tests relying on the `isVueInstance` assertion provide little to no value. We suggest replacing them with purposeful assertions.
+
+To keep these tests, a valid replacement for `isVueInstance()` is a truthy assertion of `wrapper.find(...).vm`.
 :::
 
 Assert every `Wrapper` in `WrapperArray` is Vue instance.
diff --git a/docs/api/wrapper-array/setMethods.md b/docs/api/wrapper-array/setMethods.md
@@ -1,7 +1,13 @@
 ## setMethods
 
-::: warning
+::: warning Deprecation warning
 `setMethods` is deprecated and will be removed in future releases.
+
+There's no clear path to replace `setMethods`, because it really depends on your previous usage. It easily leads to flaky tests that rely on implementation details, which [is discouraged](https://github.com/vuejs/rfcs/blob/668866fa71d70322f6a7689e88554ab27d349f9c/active-rfcs/0000-vtu-api.md#setmethods).
+
+We suggest rethinking those tests.
+
+To stub a complex method extract it from the component and test it in isolation. To assert that a method is called, use your test runner to spy on it.
 :::
 
 Sets `Wrapper` `vm` methods and forces update on each `Wrapper` in `WrapperArray`.
diff --git a/docs/api/wrapper/emittedByOrder.md b/docs/api/wrapper/emittedByOrder.md
@@ -1,6 +1,6 @@
 ## emittedByOrder
 
-::: warning
+::: warning Deprecation warning
 `emittedByOrder` is deprecated and will be removed in future releases.
 
 Use `wrapper.emitted` instead.
diff --git a/docs/api/wrapper/find.md b/docs/api/wrapper/find.md
@@ -1,6 +1,6 @@
 ## find
 
-::: warning
+::: warning Deprecation warning
 Using `find` to search for a Component is deprecated and will be removed. Use `findComponent` instead.
 :::
 
diff --git a/docs/api/wrapper/findAll.md b/docs/api/wrapper/findAll.md
@@ -1,6 +1,6 @@
 ## findAll
 
-::: warning
+::: warning Deprecation warning
 Using `findAll` to search for Components is deprecated and will be removed. Use `findAllComponents` instead.
 :::
 
diff --git a/docs/api/wrapper/isEmpty.md b/docs/api/wrapper/isEmpty.md
@@ -1,6 +1,6 @@
 ## isEmpty
 
-::: warning
+::: warning Deprecation warning
 `isEmpty` is deprecated and will be removed in future releases.
 
 Consider a custom matcher such as those provided in [jest-dom](https://github.com/testing-library/jest-dom#tobeempty).
diff --git a/docs/api/wrapper/isVisible.md b/docs/api/wrapper/isVisible.md
@@ -1,6 +1,6 @@
 ## isVisible
 
-::: warning
+::: warning Deprecation warning
 `isVisible` is deprecated and will be removed in future releases.
 
 Consider a custom matcher such as those provided in [jest-dom](https://github.com/testing-library/jest-dom#tobevisible).
diff --git a/docs/api/wrapper/isVueInstance.md b/docs/api/wrapper/isVueInstance.md
@@ -1,7 +1,11 @@
 ## isVueInstance
 
-::: warning
+::: warning Deprecation warning
 `isVueInstance` is deprecated and will be removed in future releases.
+
+Tests relying on the `isVueInstance` assertion provide little to no value. We suggest replacing them with purposeful assertions.
+
+To keep these tests, a valid replacement for `isVueInstance()` is a truthy assertion of `wrapper.find(...).vm`.
 :::
 
 Assert `Wrapper` is Vue instance.
diff --git a/docs/api/wrapper/name.md b/docs/api/wrapper/name.md
@@ -1,6 +1,6 @@
 ## name
 
-::: warning
+::: warning Deprecation warning
 `name` is deprecated and will be removed in future releases.
 :::
 
diff --git a/docs/api/wrapper/overview.md b/docs/api/wrapper/overview.md
@@ -1,6 +1,6 @@
 ## overview
 
-::: warning
+::: warning Deprecation warning
 `overview` is deprecated and will be removed in future releases.
 :::
 
diff --git a/docs/api/wrapper/setMethods.md b/docs/api/wrapper/setMethods.md
@@ -1,7 +1,13 @@
 ## setMethods
 
-::: warning
+::: warning Deprecation warning
 `setMethods` is deprecated and will be removed in future releases.
+
+There's no clear path to replace `setMethods`, because it really depends on your previous usage. It easily leads to flaky tests that rely on implementation details, which [is discouraged](https://github.com/vuejs/rfcs/blob/668866fa71d70322f6a7689e88554ab27d349f9c/active-rfcs/0000-vtu-api.md#setmethods).
+
+We suggest rethinking those tests.
+
+To stub a complex method extract it from the component and test it in isolation. To assert that a method is called, use your test runner to spy on it.
 :::
 
 Sets `Wrapper` `vm` methods and forces update.
diff --git a/packages/shared/merge-options.js b/packages/shared/merge-options.js
@@ -35,7 +35,10 @@ export function mergeOptions(
     [key: string]: Function
   })
   if (methods && Object.keys(methods).length) {
-    warnDeprecated('overwriting methods via the `methods` property')
+    warnDeprecated(
+      'overwriting methods via the `methods` property',
+      'There is no clear migration path for the `methods` property - Vue does not support arbitrarily replacement of methods, nor should VTU. To stub a complex method extract it from the component and test it in isolation. Otherwise, the suggestion is to rethink those tests'
+    )
   }
 
   const provide = (getOption(options.provide, config.provide): Object)
diff --git a/packages/test-utils/src/wrapper.js b/packages/test-utils/src/wrapper.js
@@ -632,7 +632,10 @@ export default class Wrapper implements BaseWrapper {
    * @deprecated
    */
   setMethods(methods: Object): void {
-    warnDeprecated(`setMethods`)
+    warnDeprecated(
+      `setMethods`,
+      `There is no clear migration path for setMethods - Vue does not support arbitrarily replacement of methods, nor should VTU. To stub a complex method extract it from the component and test it in isolation. Otherwise, the suggestion is to rethink those tests`
+    )
 
     if (!this.vm) {
       throwError(`wrapper.setMethods() can only be called on a Vue instance`)
diff --git a/test/specs/config.spec.js b/test/specs/config.spec.js
@@ -144,4 +144,58 @@ describeWithShallowAndMount('config', mountingMethod => {
       )
     })
   })
+
+  describe('methods overriding deprecation warning', () => {
+    const expectedErrorMessage = `There is no clear migration path`
+    const Component = {
+      template: '<div></div>',
+      methods: {
+        foo() {}
+      }
+    }
+
+    it('should show warning for options.methods if config is enabled', () => {
+      config.showDeprecationWarnings = true
+
+      mountingMethod(Component, {
+        methods: { foo: () => {} }
+      })
+
+      expect(console.error).to.be.calledWith(
+        sandbox.match(expectedErrorMessage)
+      )
+    })
+
+    it('should not show warning for options.methods if config is disabled', () => {
+      config.showDeprecationWarnings = false
+
+      mountingMethod(Component, {
+        methods: { foo: () => {} }
+      })
+
+      expect(console.error).not.to.be.calledWith(
+        sandbox.match(expectedErrorMessage)
+      )
+    })
+
+    it('should show warning for setMethods if config is enabled', () => {
+      config.showDeprecationWarnings = true
+
+      mountingMethod(Component).setMethods({ foo: () => {} })
+
+      expect(console.error).to.be.calledWith(
+        sandbox.match(expectedErrorMessage)
+      )
+    })
+
+    it('should not show warning for setMethods if config is disabled', () => {
+      config.showDeprecationWarnings = false
+
+      mountingMethod(Component).setMethods({ foo: () => {} })
+
+      expect(console.error).not.to.be.calledWith(
+        sandbox.match(expectedErrorMessage)
+      )
+    })
+  })
 })
