bookshelf icon indicating copy to clipboard operation
bookshelf copied to clipboard

buildListItem always returns NaN as finishDate and E13C6 has a bug in test

Open milamer opened this issue 5 years ago • 6 comments

Hey Kent I found two interessting bugs, that together make the test in exercise 13 work.

Generate List Items

In buildListItem, the call to faker.date.between uses a number as the startDate, but in contrast to the type definitions a number always leads to an invalid date and therefore a finishDate of NaN. (Date.parse is used in date.between)

This issue is also in the main branch and fixing this issue will make the next issue throw an error.

Exercise 13 (Integration Testing) Extra Credit 6

While testing can mark a list item as read the list item is updated in the DB, but the app is already rendered, therefore the updated list item will never be fetched.

Possible solutions:

  • We could refetch the query with the query key
  • Create the user, book, and listitem before the render call
  • Change the renderBookScreen function so that it will take overrides instead of replacements (but only create a list item if listItem is not null). As we test the book screen we will need a user and a book anyway.
async function renderBookScreen({user, book, listItem} = {}) {
  user = await loginAsUser(user)
  book = await booksDB.create(buildBook(book))
  if (listItem !== null) {
    listItem = await listItemsDB.create(buildListItem({...listItem, owner: user, book}))
  }
  ...
}

But all those changes will lead to the video Create Component-Specific Utility - Extra Credit Solution 06 to be incorrect.

milamer avatar Oct 27 '20 08:10 milamer

Interesting! Thank you for digging into this!

So yeah, this is a bit of a bummer. The correct fix is simple, but will require a slightly different approach to the test. Here's the fix:

- finishDate = faker.date.between(startDate, new Date()).getTime(),
+ finishDate = faker.date.between(new Date(startDate), new Date()).getTime(),

The problem is this will require changing the test a bit which would require I re-record the video. Not a huge deal, but a bit of a bummer.

kentcdodds avatar Oct 27 '20 15:10 kentcdodds

Here's the update that I think should be made to the test for this:

+ const user = await loginAsUser()
+ const book = await booksDB.create(buildBook())
+ const listItem = await listItemsDB.create(
+   buildListItem({owner: user, book, finishDate: null}),
+ )
+ await renderBookScreen({user, book, listItem})
+
- const {listItem} = await renderBookScreen()
-
- // set the listItem to be unread in the DB
- await listItemsDB.update(listItem.id, {finishDate: null})

I'm going to give this some thought. I think I'll need to rerecord the video before I make these changes.

Thanks again for reporting this!

kentcdodds avatar Oct 27 '20 15:10 kentcdodds

@all-contributors please add @milamer for bugs

kentcdodds avatar Oct 27 '20 15:10 kentcdodds

@kentcdodds

I've put up a pull request to add @milamer! :tada:

allcontributors[bot] avatar Oct 27 '20 15:10 allcontributors[bot]

I've updated the main branch to have this correction. An update to the exercise will come in the future.

kentcdodds avatar Oct 27 '20 15:10 kentcdodds

The video that needs an update is this one: https://epicreact.dev/modules/build-an-epic-react-app/integration-testing-extra-credit-solution-06

It's a relief that only one video is impacted, but a bummer that it's one of the longer ones 😅 I'll put this on my list though. Thanks again!

kentcdodds avatar Oct 27 '20 15:10 kentcdodds