Update toHaveBeenCalledAfter to fail if second mock is never called

[theryansmee]

Update toHaveBeenCalledAfter to fail if second mock is never called Update tests accordingly

✅ Closes #379

What

Update toHaveBeenCalledOnce predicate to ensure tests fail if first call is not made

Why

• In Why toHaveBeenCalledAfter does not check that the mock has been called? #379 it is agreed that toHaveBeenCalledOnce should also ensure that toHaveBeenCalled is also true

Notes

Housekeeping

• Unit tests
• Documentation is up to date
• No additional lint warnings
• Typescript definitions are added/updated where relevant

[codecov]

Codecov Report

Merging #420 (cb8407f) into main (7937b08) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #420   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           71        71           
  Lines          575       575           
  Branches       146       148    +2     
=========================================
  Hits           575       575           

Impacted Files	Coverage Δ
src/matchers/toHaveBeenCalledAfter.js	100.00% <100.00%> (ø)
src/matchers/toHaveBeenCalledBefore.js	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7937b08...cb8407f. Read the comment docs.

[keeganwitt]

Do you think we should also change toBeBefore to fail if the second mock is never called?

[theryansmee]

Do you think we should also change toBeBefore to fail if the second mock is never called?

I have been contemplating the same thing.

I don't think it's as necessary as the change to toBeAfter, but i'm happy to take a look in the interest of keeping the behaviour similar between the two?

[keeganwitt]

Do you think we should also change toBeBefore to fail if the second mock is never called?

I have been contemplating the same thing.

I don't think it's as necessary as the change to toBeAfter, but i'm happy to take a look in the interest of keeping the behaviour similar between the two?

That's what I was thinking was to keep the behavior similar. I was on the fence before, but the more I started thinking about it, the more I think it makes sense. If we don't check that the second call happened, there's no difference between toHaveBeenCalledBefore and toHaveBeenCalled() in cases where the second call didn't happen.

Another thing I'm thinking about is whether we should make this behavior configurable too, like

diff --git a/src/matchers/toHaveBeenCalledAfter.js b/src/matchers/toHaveBeenCalledAfter.js
index 01bb87a..5aa6182 100644
--- a/src/matchers/toHaveBeenCalledAfter.js
+++ b/src/matchers/toHaveBeenCalledAfter.js
@@ -1,6 +1,6 @@
 import { isJestMockOrSpy } from '../utils';

-export function toHaveBeenCalledAfter(actual, expected) {
+export function toHaveBeenCalledAfter(actual, expected, failIfNoFirstInvocation) {
   const { printReceived, printExpected, matcherHint } = this.utils;

   if (!isJestMockOrSpy(actual)) {
@@ -13,7 +13,7 @@ export function toHaveBeenCalledAfter(actual, expected) {

   const firstInvocationCallOrder = actual.mock.invocationCallOrder;
   const secondInvocationCallOrder = expected.mock.invocationCallOrder;
-  const pass = predicate(firstInvocationCallOrder, secondInvocationCallOrder);
+  const pass = predicate(firstInvocationCallOrder, secondInvocationCallOrder, !!failIfNoFirstInvocation);

   const passMessage =
     matcherHint('.not.toHaveBeenCalledAfter') +
@@ -36,8 +36,8 @@ export function toHaveBeenCalledAfter(actual, expected) {

 const smallest = ns => ns.reduce((acc, n) => (acc < n ? acc : n));

-const predicate = (firstInvocationCallOrder, secondInvocationCallOrder) => {
-  if (firstInvocationCallOrder.length === 0) return false;
+const predicate = (firstInvocationCallOrder, secondInvocationCallOrder, failIfNoFirstInvocation) => {
+  if (firstInvocationCallOrder.length === 0) return !failIfNoFirstInvocation;
   if (secondInvocationCallOrder.length === 0) return false;

   const firstSmallest = smallest(firstInvocationCallOrder);

[theryansmee]

Do you think we should also change toBeBefore to fail if the second mock is never called?

I have been contemplating the same thing.
I don't think it's as necessary as the change to toBeAfter, but i'm happy to take a look in the interest of keeping the behaviour similar between the two?

That's what I was thinking was to keep the behavior similar. I was on the fence before, but the more I started thinking about it, the more I think it makes sense. If we don't check that the second call happened, there's no difference between toHaveBeenCalledBefore and toHaveBeenCalled() in cases where the second call didn't happen.

Another thing I'm thinking about is whether we should make this behavior configurable too, like

diff --git a/src/matchers/toHaveBeenCalledAfter.js b/src/matchers/toHaveBeenCalledAfter.js
index 01bb87a..5aa6182 100644
--- a/src/matchers/toHaveBeenCalledAfter.js
+++ b/src/matchers/toHaveBeenCalledAfter.js
@@ -1,6 +1,6 @@
 import { isJestMockOrSpy } from '../utils';

-export function toHaveBeenCalledAfter(actual, expected) {
+export function toHaveBeenCalledAfter(actual, expected, failIfNoFirstInvocation) {
   const { printReceived, printExpected, matcherHint } = this.utils;

   if (!isJestMockOrSpy(actual)) {
@@ -13,7 +13,7 @@ export function toHaveBeenCalledAfter(actual, expected) {

   const firstInvocationCallOrder = actual.mock.invocationCallOrder;
   const secondInvocationCallOrder = expected.mock.invocationCallOrder;
-  const pass = predicate(firstInvocationCallOrder, secondInvocationCallOrder);
+  const pass = predicate(firstInvocationCallOrder, secondInvocationCallOrder, !!failIfNoFirstInvocation);

   const passMessage =
     matcherHint('.not.toHaveBeenCalledAfter') +
@@ -36,8 +36,8 @@ export function toHaveBeenCalledAfter(actual, expected) {

 const smallest = ns => ns.reduce((acc, n) => (acc < n ? acc : n));

-const predicate = (firstInvocationCallOrder, secondInvocationCallOrder) => {
-  if (firstInvocationCallOrder.length === 0) return false;
+const predicate = (firstInvocationCallOrder, secondInvocationCallOrder, failIfNoFirstInvocation) => {
+  if (firstInvocationCallOrder.length === 0) return !failIfNoFirstInvocation;
   if (secondInvocationCallOrder.length === 0) return false;

   const firstSmallest = smallest(firstInvocationCallOrder);

Yeah thats a really good shout!
I also think that failIfNoFirstInvocation should default to true so the user only passes it if they don't care about both mocks being called? (The only draw back being that this would then be a breaking change as currently tests pass without the second call).

What are your thoughts?

Also, I have made the requested changes to toHaveBeenCalledBefore

[theryansmee]

@keeganwitt - When you get a chance, have a look at my last commit and let me know what you think of my implementation of the discussed param

[keeganwitt]

I also think that failIfNoFirstInvocation should default to true so the user only passes it if they don't care about both mocks being called? (The only draw back being that this would then be a breaking change as currently tests pass without the second call).

Yea, that is a better way to do it. I'm still mulling the overall idea over, particularly the point you raise about it being a breaking change, but I'm thinking this is the right approach.