Closes #253: Add change matchers, .toChange(), .toChangeBy(), .toChangeTo()

[MrLeebo]

Adds three change matchers, .toChange(), .toChangeBy(), and .toChangeTo(), based on #253 (comment).

// lunch.spec.ts
// totally fake example test script

test("adds lunch item", () => {
  expect(() => {
    lunch.add("pizza");
  }).toChange(() => lunch.count())
})

test("adding lunch items adds to cost", () => {
  lunch.add("salad"); // $5
  expect(() => {
    lunch.add("pizza"); // $7
    lunch.add("soda"); // $3
  }).toChangeBy(() => lunch.totalCost(), 10);
});

test("kids lunch comes with a free drink", () => {
  lunch.add("kids grilled cheese")
  expect(() => {
    lunch.add("kids drink")
  }).not.toChange(() => lunch.totalCost());
});

test("canceling order also clears the bill", () => {
  lunch.add("pizza");
  expect(() => {
    lunch.cancel();
  }).toChangeTo(() => lunch.totalCost(), 0)
});

test("a nice lunch at taco bell", () => {
  expect(() => {
    lunch.add("14 burritos"); // $2
  }).toChangeTo(() => lunch.totalCost(), 28);
});

Instead of one matcher with variant modes, I split it into three separate matchers so that the typescript definitions would not be overloaded.

    /**
     * Use `.toChange` when checking if a value has changed.
     * @example
     * expect(() => value++).toChange(() => value);
     */
    toChange<E = unknown>(checker: () => E): R;

    /**
     * Use `.toChangeBy` when checking if a value changed by an amount.
     * @example
     * expect(() => value--).toChangeBy(() => value, -1);
     */
    toChangeBy(checker: () => number, by?: number): R;

    /**
     * Use `.toChangeTo` when checking if a value changed to a specific value.
     * @example
     * expect(() => Model.deleteAll()).toChangeTo(() => Model.count(), 0);
     */
    toChangeTo<E = unknown>(checker: () => E, to?: E): R;

What

This is an implementation of the matchers requested in #253 with unit tests and documentation added.

Each of these matchers accept a checker callback function that it calls before and after invoking the mutator function. The checker function should access some mutable state value that can be modified by the code under test.

.toChange(checker)

Expect the state to have changed, or not to have changed when used with .not, by comparing to the before value with ===.

.toChangeBy(checker, by)

Expect the state to change by the given amount, which must be a number.

.toChangeTo(checker, to)

Expect the state to change to the given value.

Why

It's pretty common to test something that should make a change in your app, such as adding or removing an item from a list, then having an assertion to check that the count has changed. Often when we write these tests, we simply check the count of the list after the change.

// before
test("clears the list", () => {
  tasks.deleteAll();
  expect(tasks.count()).toEqual(0);
});

This test can pass if the function works as advertised, but it can also pass if the list was empty from the start. Change matchers can assert that the state was changed by the logic you're testing.

// after
test("clears the list", () => {
  expect(() => tasks.deleteAll()).toChangeTo(() => tasks.count(), 0)
})

Notes

First contribution 👋

Housekeeping

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

[MrLeebo]

It looks like the <TestFile> entries in the docs mdx file can't find any of the new matchers. Is there something I need to update in order to make that work?

[MrLeebo]

@keeganwitt is there anything I can do to help progress this forward?

[keeganwitt]

@keeganwitt is there anything I can do to help progress this forward?

Thanks for the PR! I'll try to take a look this week.

[codecov]

Codecov Report

Merging #521 (0841424) into main (bd824cd) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #521   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           72        77    +5     
  Lines          594       642   +48     
  Branches       151       161   +10     
=========================================
+ Hits           594       642   +48     

Impacted Files	Coverage Δ
src/matchers/toChange.js	100.00% <100.00%> (ø)
src/matchers/toChangeBy.js	100.00% <100.00%> (ø)
src/matchers/toChangeTo.js	100.00% <100.00%> (ø)
test/fixtures/counter.js	100.00% <100.00%> (ø)
src/matchers/toBeInRange.js	100.00% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[keeganwitt]

I haven't forgotten about you. I've been ill the last several days.

[keeganwitt]

I'm kinda questioning whether there's value in adding toChange(). Can you think of use cases where that'd actually be useful? I know it's what rspec has, just thinking aloud.

[keeganwitt]

I've wondered if this should be assuming numbers. toChangeBy() obviously only works for numbers, but one could imagine something like

expect(() =>menu.updateDailySpecial(tuesday)).toChangeTo(() => menu.dailySpecial, 'Meatloaf');

[MrLeebo]

I'm kinda questioning whether there's value in adding toChange(). Can you think of use cases where that'd actually be useful? I know it's what rspec has, just thinking aloud.

I think toChange() is most useful when inverted with .not:

// first bowl is free
expect(() => {
  lunch.add("chips and salsa")
}).not.toChange(() => lunch.totalCost())

// but the second bowl will cost you
expect(() => {
  lunch.add("chips and salsa")
}).toChange(() => lunch.totalCost())

[keeganwitt]

I'm kinda questioning whether there's value in adding toChange(). Can you think of use cases where that'd actually be useful? I know it's what rspec has, just thinking aloud.

I think toChange() is most useful when inverted with .not:

// first bowl is free
expect(() => {
  lunch.add("chips and salsa")
}).not.toChange(() => lunch.totalCost())

// but the second bowl will cost you
expect(() => {
  lunch.add("chips and salsa")
}).toChange(() => lunch.totalCost())

That's fair.

[keeganwitt]

I'm still on the fence about adding these. They do improve readability, but they don't reduce any setup. There's not any use case I can think of that is made easier by having these.

This project tries to add matches that provide a certain "threshold" of value and not to add every conceivable matcher. I'm still thinking about whether this feature has reached that threshold.

[MrLeebo]

Naturally I think it brings enough value, I wouldn't have opened the PR if I didn't. I expect that I have an argument that would convince you, but with Thanksgiving preparations I don't have a lot of time to write up a full explanation, so I'll put in a brief, bulleted version for now and maybe provide more context if needed after the holiday.

• The "easy" solution is to expect() the state before and after the state change.
• This works well for basic unit tests, but doesn't necessarily scale as well for integrations / systems tests.
• In those contexts, you're usually not setting up the full app state within the context of every test, you're working from a shared set of data fixture files or seed scripts.
• So instead of

expect(Product.count()).toEqual(0)
Product.create()
expect(Product.count()).toEqual(1)

What you're writing is more like

const initialCount = Product.count()
Product.create()
expect(Product.count()).toEqual(initialCount + 1)

• Reason is that since many tests share the same data fixtures, another test could add more products for other tests, so hard coded counts in this test will cause it to be brittle to changes in the fixtures.

• Now, this code still isn't so bad, but sometimes these actions have second order effects, like updating the Product's price will update the product model, but it may also touch on other models (like an change audit / price history table that automatically gets an insert when the price changes)

• It would be annoying to have to take the "initialCount" of every nested relation in each test to use the "easy" solution

• Should a single test cover so much code? Again, these are integration tests not unit tests, they are meant to cover how the app works when all the pieces are brought together.

• See "unit tests pass meme"

Source

[keeganwitt]

I'm not trying to shoot this idea down. I just want to be thoughtful before proceeding. I'm also with family this week. I just wanted to let you know I was still mulling this over. The short version of what's giving me pause is

const counter = new Counter();
expect(counter.increment).toChange(counter.count);

// vs

const counter = new Counter();
const originalCount = counter.count;
counter.increment();
expect(counter.count).not.toBe(originalCount);

const counter = new Counter();
expect(counter.increment).toChangeBy(counter.count);

// vs

const counter = new Counter();
const originalCount = counter.count;
counter.increment();
expect(counter.count - originalCount).toEqual(originalCount + 1);

const counter = new Counter();
expect(counter.increment).toChangeTo(counter.count, 1);

// vs

const counter = new Counter();
counter.increment();
expect(counter.count).toEqual(1);

The new matchers save a tiny bit of code, and it's more readable, but one could ask "how far should this go?" Should we make function wrappers be added for other existing matchers? e.g. testing that a value was added/removed to an array after a function call, testing that a date was mutated by a certain amount after a function call, testing that the type of something changed to a string/boolean/number etc after a function call, testing that a number is/isn't negative after a function call, etc. Lots of matchers could have a version added that does its check after invoking a function.

But I'll give what you've written some more thought.