thoughtbot / fishery

A library for setting up JavaScript objects as test data
MIT License
877 stars 36 forks source link

Fixing onCreate hooks being left unresolved #45

Closed vpillinger closed 3 years ago

vpillinger commented 3 years ago

This fixes a significant issue introduced in #40.

The purpose of onCreate() hooks is to allow users to run asynchronous code (ex. saving a model to a database).

However, the current implementation does not resolve the onCreate() hooks, which means that code like the following is still impossible to test:

test('Gets the correct user', async () => {
  await userFactory.create();
  const user = await User.query().findById(1); // Will not find the user, because userFactory.create() resolves without waiting for .then() functions to resolve
  expect(user.id).toBe(1);
});

Additionally, if the onCreate() hook throws an exception, it does not catch that exception which leaves dangling promise rejections.

This PR fixes both issues.

ObliviousHarmony commented 3 years ago

Howdy @vpillinger, great catch! Thank you very much for cleaning up my mess 😀

I believe that ultimately the defect in the original PR is just that the promise returned here is being discarded. All of the onCreate hooks are being registered with the first promise instead of functioning as a chain. My thinking is that changing the line into created = created.then(onCreate); would sufficiently resolve all of the noted problems without complicating the code.

I don't have too many thoughts on the rest of the PR though I do question the value added by some of the test related changes.

stevehanson commented 3 years ago

@vpillinger many thanks for spotting this issue and opening this PR. I ended up merging #46 instead since the change was a little bit simpler. Cheers!