minesweeper: update tests to version 1.1.0 #1062
Conversation
| # self.assertEqual(board([]), []) | ||
|
|
||
| # def test_no_columns(self): | ||
| # self.assertEqual(board([""]), [""]) |
There was a problem hiding this comment.
Can you please give a more detailed explanation for why these tests are disabled? I don't like setting a precedent for skipping/disabling canonical tests.
There was a problem hiding this comment.
Originally this was because the implementation of the board class uses the verify_board function to reject boards without the border or no rows/columns but the canonical data looks like it expects a return of the board as is for both of these cases.
If we'd be okay with it, I can reenable both of these cases but convert both into checks that the board class raises a ValueError in response to the board type instead.
There was a problem hiding this comment.
I would prefer that you re-enable the disabled test cases, but instead of converting the tests, can we simply do away with the verify_board method? There is no requirement in either the canonical data or the description that the solution needs to validate the board; it is merely meant to convert it.
There was a problem hiding this comment.
We could remove the verify_board method from the example implementation, but I think it would necessitate removing a few of the additional test cases as well (the final 3 that test malformed boards).
I think it may be desirable to have a verify_board method though. If there's no restriction on the input being a well formed board especially cases with rows/columns of differing sizes it may be hard to decide what's truly adjacent to a mine thus complicating generating output.
It also seems like the no row/no column board with no border might not be valid given that all the boards in the problem description and old test set always include a drawn border.
There was a problem hiding this comment.
Personally, these don't look like valid boards to me, given the borders present in all other examples, so I would expect these to raise ValueErrors. Ultimately, I don't mind whether these tests are modified or removed - I don't think that they add anything to what we already have.
@cmccandless, I like the inclusion of board verification, and I don't think that it makes sense to remove it in order to better match the canonical data for two trivial cases.
| @@ -10,114 +10,158 @@ | |||
|
|
|||
| from minesweeper import board | |||
|
|
|||
| # Tests adapted from `problem-specifications//canonical-data.json` @ v1.0.0 | |||
|
|
|||
There was a problem hiding this comment.
@smalley, can you move the version comment down by a line for consistency? (ie. two blank lines before, one after)
There was a problem hiding this comment.
Sure, no problem at all @N-Parsons, I can make that change and push an update.
|
Having looked at this again, I noticed that the canonical data doesn't have the borders. @cmccandless, what are your thoughts on removing the borders? |
Add missing test version comment. Add ordered tests with names matching canonical data descriptions. Disable empty board tests which are incompatible with board class. Move selected old tests to additional tests section. Convert no row/column tests to expect validation to raise ValueError. Closes exercism#998
|
I updated this pull request to convert the first two tests to expect ValueError to be raised and uncommented them. If we want to remove the border or convert the no row/ no column tests to have borders we can definitely do those as well. |
|
@N-Parsons if the canonical data does not have borders, then I don't think the tests should have them. If this change were made, do we have any reason to keep the board validation? |
|
Ok, I agree that we should remove the borders. I think that without the borders, the validation becomes too trivial to really worry about making people implement, so I think that it can be removed as well. @smalley, are you happy to make these changes? |
|
@N-Parsons sure, I can work on making those changes to the boards and remove the validation function |
|
@N-Parsons |
exercises/minesweeper/example.py
Outdated
| @@ -31,9 +33,3 @@ def verify_board(inp): | |||
| cset.update(r) | |||
| if cset - set('+- *|'): | |||
There was a problem hiding this comment.
The second set should be set(" *") now, since the other elements were for the border.
|
Thanks @smalley, I've left a comment about a very minor issue the example solution (the set of valid characters needs updating). Once that's fixed, I think this will be ready to merge. @cmccandless, can you review this again? |
Update test data to borderless format which matches canonical data format. Change board verification to not test for borders and default example to properly return a completely empty board and account for new index when there is no border.
|
Looks good. Thanks, @smalley! |
Add missing test version comment.
Add ordered tests with names matching canonical data descriptions.
Disable empty board tests which are incompatible with board class.
Move selected old tests to additional tests section. Closes #998