Skip to content

Conversation

@Snaptags
Copy link
Contributor

@Snaptags Snaptags commented Jan 7, 2021

It might be nice to verify the button actually increases the existing value. If you start to implement the extra credit requirements you might accidentally set the count to "step". Then the test would still be green, it only fails when you try to click the button again…

Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Snaptags, could you update your branch to the latest? This test changed quite a bit. Also, I'm not sure what these changes do to help. Could you elaborate?

@Snaptags
Copy link
Contributor Author

Snaptags commented Jan 8, 2021

Sorry, did not upgrade my fork first 😞

The test gives a deceptive feeling of safety when starting the extra credit 1 exercise: If you just apply the changes suggested by the instructions, the test immediately is green again. Because, accidentally, replacing the state by the step (1 in this case) is a valid result.

const [count, changeCount] = React.useReducer(countReducer, initialCount)
const increment = () => changeCount(step)

But people have to change the reducer implementation, too. By clicking the increase button twice the test makes sure that you don't mess up the reducer while refactoring. And to make sure you actually can increase the counter more than once.

Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes total sense. Thank you!

@kentcdodds kentcdodds merged commit 3966f8c into epicweb-dev:main Jan 8, 2021
@kentcdodds
Copy link
Member

@all-contributors please add @Snaptags for tests

@allcontributors
Copy link
Contributor

@kentcdodds

I've put up a pull request to add @Snaptags! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants