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

Extend pangram test suite #294

Closed
wants to merge 1 commit into from
Closed

Extend pangram test suite #294

wants to merge 1 commit into from

Conversation

victorbrender
Copy link

Some implementations use the "/\w/" regex to match characters and add them to an array and then check that the size of the array is equal to 26. These implementations erroneously pass the current test suite.

Some implementations use the "/\w/" regex to match characters and add them to an array and then check that the size of the array is equal to 26. These implementations erroneously pass the current test suite.
@Cohen-Carlisle
Copy link
Member

Thanks for pointing this out!

This exercise is generated from common metadata, so the ultimate fix is to update that metadata. I'd like to get the metadata fixed and then regenerate this exercise instead of doing a one off fix here. If you are up for it, you could open an issue or a PR on the x-common repo.

Should the test's name be a bit more clear about exactly what its testing? For example, would test_pangram_with_underscore_word_separators be better than test_pangram_with_underscore_character? I'm leaning towards a yes, but I'm not sure.

The name test_pangram_with_sentence_containing_numbers shouldn't say pangram as the input in this case isn't. Probably something along the lines of test_letters_replaced_with_numbers.

end

def test_pangram_with_sentence_containing_numbers
str = 'the qu1ck br0wn fox jumps over the lazy dog'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid 0 and 1 as they are too close in some fonts to l and O. When possible, use distinctively shaped numbers, rather than the possibly visually ambiguous ones.

This becomes aggravated especially when used in places where those ambiguities are replacements of letters that are so similar.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The purpose of this test is to replace the letters with a corresponding number and make sure the solution does not think the string is a pangram. With this in mind, I think its ok. Especially if the method name were updated to something like test_letters_replaced_with_numbers.

At worst its an exercise in paying attention to detail, which is a good skill to have as a programmer.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that the purpose of the test?

I don't think it is, and if so, it is not expressed clearly.

It appears that the purpose is that if there are 26 unique characters and yet it is missing the only letter this is missing, which happens to be an 'i', it should not be classified as a pangram.

For intent, though, we would definitely need to ask @victorbrender since he is the only one that can confirm intent, as the author.

The visual distraction of similarly shaped numbers in the place of letters is simply a distraction. Noting that the 'i' is the only thing missing is the exercise of detail attention that you speak of. It happens here regardless of the visual distraction of the numbers similarity to the letters they are replacing.

@kotp
Copy link
Member

kotp commented Apr 18, 2016

This is what I came up with that addresses test naming and the potential for misunderstanding the intent of the PR.

def test_pangram_with_underscore_character
  str = 'the quick brown_fox jumps over the lazy dog'
  assert Pangram.is_pangram?(str)
end

def test_not_a_pangram_with_26_unique_characters
  str = '0123456789klmnOPQRstuvWXYz'
  refute Pangram.is_pangram?(str)
end

def test_pangram_with_string_containing_numbers_which_is_otherwise_valid
  str = '0123456789abcdefghijklmnOPQRstuvWXYz'
  assert Pangram.is_pangram?(str)
end

@victorbrender what do you think? Does it address what your concerns are? If so, can you update your PR?

@Cohen-Carlisle
Copy link
Member

I took the liberty of creating exercism/problem-specifications#228 to fix this for all languages, as well as address another gap in testing noted there.
@kotp @victorbrender input welcome

@kotp
Copy link
Member

kotp commented Apr 18, 2016

@kytrinyx
Copy link
Member

FYI @kotp and @Cohen-Carlisle I thought all track maintainers had commit on the x-common repository, but I failed at UX, and so everyone had "read" access. Great. On a public repo :) Anyway, this is fixed, and if y'all are happy with exercism/problem-specifications#22, then either of you could go ahead and merge that.

@kotp
Copy link
Member

kotp commented Apr 23, 2016

Thanks @victorbrender for the help in getting this worked out. Please do let us know if we missed the mark.

@kotp kotp closed this Apr 23, 2016
gchan pushed a commit to gchan/xruby that referenced this pull request Oct 18, 2016
Giving a new rule where a non-prime number is expected to be a factor,
this should not fail.

fixes exercism#294
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.

None yet

4 participants