Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add experience part 1 #448
add experience part 1 #448
Changes from 6 commits
d96a158
00d7a56
3296199
91583f7
fd88b39
f791e83
89bb2ee
1f5e5f7
023e1c7
0556d30
de09e52
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally this unit test would not be concerned with implementation details like we're doing here. Could this test be changed to examine
gameState
to detect whether the level-up was processed instead?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i have mixed feelings about this. not strong enough that I won't change it, but I do want to offer my perspective. i did things this way because I see this test as a unit test, and if it were to check that a level changed it would have to know about game logic conditions that cause the level to change (i.e. how much experience it takes to get from one level to another). the way i see,
addExperience
is only concerned with making sure thatprocessLevelUp
is invoked, which is what mocking it and checking for it to be called does. i think the place for doing a test on game logic belongs elsewhere, which is what might be missing from this PR, but i haven't found that spec yet 😄 .There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is where our views diverge. Based on the semantics and API of this reducer, I would assume that
addExperience
only cares about adding experience. The fact that level-ups are processed within this reducer seems like an implementation detail that could conceivably be handled outside of this function (though I'm not suggesting we do that).We definitely do want to test that adding experience causes a level up, but perhaps this isn't the appropriate place to validate that? Specifically, I'm thinking that it may be better to validate that gaining experience results in a level-up in a higher-level React Test Library test. I'm thinking something similar to this:
farmhand/src/game-logic/item-sell.test.js
Lines 9 to 29 in f791e83
I realize that this is expanding the scope of what you set out to do here, and the current state isn't something I'd block this PR over or anything. It's just a difference of opinion. If you feel good about what we have here, then I'm okay with sticking with it. Just let me know which way you'd like to go. 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i actually think i'm with you on processing level ups as being the thing that is out of place here. so i think i'll remove that. it definitely feels like an unexpected side effect not that you've mentioned it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok i've removed it. i see that
sellItem
is what actually doesprocessLevelUp
but I don't see any tests in there explicitly checking for level up. i always forget thatlevel
isn't part of farmhand state and that it's calculated when needed so testing that a level up was achieved is trickier than i expected (at least, without just mocking and checking for the call.) if you have suggestions on how to test it let me know and i am happy to add it to this PRThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making that change!
If we're speaking in terms of React Testing Library tests, I'd approach this by checking to see if the level up notification text is present on the page:
farmhand/src/templates.js
Lines 184 to 214 in d37d410
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ahh yes that makes sense. the
sellItem
test doesn't seem like the right place in that case (it isn't doing any rendering). i suppose we could check the notifications in state from there though?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it might be worth spinning up a new test file sibling to
src/game-logic/item-sell.test.js
just for testing level-ups. This is game logic that cuts across functional units, so an integration test seems appropriate.