Skip to content
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

Ensure there is a unit test for every property. #53

Merged

Conversation

pak21
Copy link

@pak21 pak21 commented Feb 5, 2019

Too long a day at work to think about the big PR, so here's a simple one: ensure that there is a unit test for every property which exists on the BoardGame object. This is particularly important if we're going to refactor any of this in the near future :-)

Interestingly, this reveals what might actually be a bug: the BoardGame.users_wanting, BoardGame.users_wishing and BoardGame.users_commented properties are trying to fetch data from self._data rather than self._stats and always seem to be None. If you agree this is a bug, I'll do a separate PR for that.

@lcosmin lcosmin merged commit a3fea0e into lcosmin:develop Feb 6, 2019
@lcosmin
Copy link
Owner

lcosmin commented Feb 6, 2019

Yes, it's a bug. It has gone unnoticed because at the beginning the tests were done using the real BGG API and I just ignored values which could change from call to call (probabilities, averages, etc). After switching to mocked responses from the API, I simply forgot to add tests for everything.

I guess that, since nobody reported this issue, nobody uses those properties 😄

Yeah, sure, send a PR, I'll merge it 👍

@pak21 pak21 mentioned this pull request Feb 11, 2019
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