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

Word Count: Apostrophe bug that users' submissions could plausibly include, that current tests won't detect #1977

Open
rneilsen opened this issue Mar 4, 2022 · 3 comments

Comments

@rneilsen
Copy link
Contributor

rneilsen commented Mar 4, 2022

The current test suite for the word-count exercise tests for words containing apostrophes, but the only examples used in the test cases themselves are words that only have one letter after the apostrophe (specifically, "can't" and "don't").

As a consequence, if a user implements code that doesn't correctly handle words with more than one letter after the apostrophe (like I did, with a regex that looked like /([a-z]+'[a-z]| ... )/) the tests will not identify the problem. There aren't a lot of English examples where this might cause miscounts - the only one I could think of was "m'lord"/"m'lady", other than fantasy stuff like "bat'leth"/"bat'leths" - but the thought process that got me there seems pretty plausible as a mistake others might make (I looked at the test cases, saw "don't" and "can't", and thought of "I'm" and "he's", and didn't think of "you're" and "they'll"), so it seems like something worth checking for in the test cases.

Seems like a pretty easy fix to update the "with apostrophes" test case to include a word like "you're" or whatever, I'm happy to submit a PR if it's agreed as worthwhile.

@ErikSchierboom
Copy link
Member

Seems like a pretty easy fix to update the "with apostrophes" test case to include a word like "you're" or whatever, I'm happy to submit a PR if it's agreed as worthwhile.

I like the suggestion. My only remark would be that it would have to be a new test case as test cases are immutable. We can discuss whether or not we want the new test case to supersede (reimplements in problem-specifications parlance) the current test case (I think we might).

@rneilsen
Copy link
Contributor Author

rneilsen commented Mar 7, 2022

Dang it, there's always a catch. Is there a documented procedure for creating a new test to replace an old one? I had a look but couldn't find anything - this section of CONTRIBUTING.md just says to submit a PR for the JSON file, but it doesn't give any details.

I could probably write up a test, give it a new uuid, and submit a PR with the new test where the old one was, but I don't want to waste anyone else's time if there's a proper procedure to go through that I should be following.

@ErikSchierboom
Copy link
Member

@rneilsen There is a section in the README that explains how to do this: https://github.com/exercism/problem-specifications#changing-tests

rneilsen added a commit to rneilsen/problem-specifications that referenced this issue Mar 7, 2022
As per [issue 1977](exercism#1977), if the user's code was unable to correctly handle words with multiple letters after an apostrophe (a plausible error one might make with a regex, for example), the previous version of the "with apostrophes" test, which only included the word "don't", would not detect the bug (and none of the other tests would find it either).

This change is a reimplement of the "with apostrophes" test to now include the word "you're" as part of the input, so if the user's code has a bug and incorrectly handles such words, the new test will fail and expose it.
kotp pushed a commit that referenced this issue Mar 8, 2022
* Re-implement "with apostrophes" test

As per [#1977], if the user's code was unable to correctly handle words with multiple letters after an apostrophe (a plausible error one might make with a regex, for example), the previous version of the "with apostrophes" test, which only included the word "don't", would not detect the bug (and none of the other tests would find it either).

This change is a re-implementation of the "with apostrophes" test to now include the word "you're" as part of the input, so if the user's code has a bug and incorrectly handles such words, the new test will fail and expose it.

[#1977]: #1977
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants