I’d like to introduce two new tests to Phone Number exercise.
This is due to the encounter during mentoring that some tests are missing.
When function input is 11-digits long (after cleaning), the first one is 1, no checks are done if the code handles properly area code’s and exchange code’s first digit.
I only need a tip how to approach adding tests in PR to be aligned with general Exercism structure. Just adding tests in test_phone-number.R or sth more is required?
Before submitting a PR, get approval for changes. And read,
Thank you, I think I went through these already during previouse contributions but reading it again is always worthy to refresh my memory.
During one of my mentoring sessions I’ve found that student uploaded exercise which gets away with the complications instructions give. That observation led me to think we need to add two tests.
We need to test that when area code or exchange code are lower than 2 in the 11-digit long number starting with “1”, the function returns NULL.
Currently student’s code can return invalid phone number for that case and pass tests.
@colinleach and @jonmcalder WDYT?
Comparing the tests in test_phone-number.R with the Exercism-wide standard in canonical-data.json, it looks like the R track has fallen out of sync in the 8 years since this exercise was implemented.
The last 2 tests in test_phone-number.R only cover 10-digit numbers.:
test_that("invalid if area code does not start with 2-9", {
expect_equal(parse_phone_number("(123) 456-7890"), NULL)
})
test_that("invalid if exchange code does not start with 2-9", {
expect_equal(parse_phone_number("(223) 056-7890"), NULL)
})
These have now been replaced by 8 tests in canonical-data.json: four for 10-digit and four for 11-digit numbers. These are the last 8 tests in that file.
Changing tests is likely to break existing community solutions, so this is a non-trivial step. If we are going to do it, I think it would be better to remove the 2 old tests and add 8 new ones matching the canonical data.
What do other people think?
I think if the exercise is out of sync, invalidating student solutions while updating it is fair. Otherwise, the track risks having the exercise become track-only, as it drifts further and further from the canonical data.
OK, let’s fix this.
@Nerwosolek, would you like to submit a PR to make the R tests match the canonical data?
I agree that it makes sense to update the tests to match the canonical data (even though it will invalidate some existing / older solutions).
For clarification: the new tests should still return NULL on failure. Unlike many system languages and the canonical data, we don’t want R to terminate with an error message every time it finds some invalid data.
Sure, I’m on it!
I’ve send a PR as requested: Fix phone number tests by Nerwosolek · Pull Request #333 · exercism/r · GitHub
Changes:
Because previous example.R didn’t pass some of new new tests,
it was changed based on my solution which already followed instructions properly. I didn’t want to rethink proposed example as a little non optimal.