word-count: added test version and updated tests to match canonical #1044
Conversation
|
I think it's better to add the failing tests but comment them for now and create an issue for fixing the example solution (you've already done that #1045) |
|
I'll go ahead and add them back in and comment them out. |
|
Didn't mean for all of those other commits to be in here. Is there a way to undo that? |
|
@jackattack24 No worries, we can squash the commits when it's time to merge the PR. |
|
@m-a-ge On updating example solution with new tests: I've been telling PR authors to update the example solution as well. For the sake of consistency, should we should update the example solution here as well? @jackattack24 Sorry, I just looked at what commits you were referring to. To fix this on your end, I would recommend the following: $ git checkout word_count_test
$ git remote add upstream https://github.com/exercism/python.git
$ git pull upstream master
$ git rebase -i masterWhen prompted, simply remove the commit lines for all commits you wish to remove, save file, and close. Then: $ git push --forceNormally, |
jackattack24
force-pushed
the
word_count_test
branch
from
f44b387 to
5a17065
Compare
| # def test_apostrophes(self): | ||
| # self.assertEqual( | ||
| # {'first': 1, "don't": 2, 'laugh': 1, 'then': 1, 'cry': 1}, | ||
| # word_count("First: don't laugh. Then: don't cry.") |
There was a problem hiding this comment.
Can you update the order of these arguments (and the ones in the rest of the test-suite) to match the assertEqual(actual, expected) format?
There was a problem hiding this comment.
I've left a comment with regard to argument ordering, but looks like your previous changes aren't here after you fixed the issue with spurious commits.
Also, I've submitted a fix to #1045 in #1059, so my suggestion would be to uncomment the new tests (that currently fail) once you've finished and you're sure there aren't other reasons for the tests to fail - the test will pass again once my PR is merged and the branch is updated, and this will be checked before merging.
There was a problem hiding this comment.
Looks good, @jackattack24.
I've left a comment regarding one of the tests though - it would be better for all of the tests to be done in the same way. Once you've updated it, this will be ready to merge.
| [2, 3], | ||
| sorted(list(word_count('go Go GO Stop stop').values())) | ||
| sorted(list(word_count('go Go GO Stop stop').values())), | ||
| [2, 3] |
There was a problem hiding this comment.
Would you mind changing the format of this test to match the other ones for consistency?
I had a poke around and it looks like it was done differently so that words could be uppercase in the list (#207), but the tests added to this track since all assume that words are normalised to lowercase. I kind of like the original idea of giving learners freedom, but I think that practical applications would normalise to lowercase if they wanted a case-insensitive count, and I can't think of a clean and robust way to implement case-insensitive testing.
|
Perfect! Thanks, @jackattack24! |
…xercism#1044) * Added version number and updated tests to match canonical data. * Changed argument order for the tests. * Updated mixed case test
Fix #1011
As I mentioned in issue #1045, there were two tests from the canonical data that failed with the example implementation, so I left them out.