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

scrabble-score: add a test for the entire alphabet #250

Merged
merged 2 commits into from
Jan 31, 2017

Conversation

juleskers
Copy link
Contributor

After passing all tests for scrabble-score, I noticed I still didn't have all letters.
Adding them to my implementation at that point of course was trivial, but since there wasn't a test, I wrote one.
Here it is offered for inclusion at the source!

so comments, suggestions, nits and other (constructive) criticism welcome;
This is only my second[1] exercism contribution, so I may need some guidance ;-)

[1] first contribution

Copy link
Member

@petertseng petertseng left a comment

Choose a reason for hiding this comment

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

Well I at least think this test is appropriate!

If you think this test would be appropriate all language tracks and not just the Rust track, then it would be great if you would add it to https://github.com/exercism/x-common/blob/master/exercises/scrabble-score/canonical-data.json as well.

One small thing before we merge it for this track! We put the #[ignore] annotation on all tests except the first, as we see in https://github.com/exercism/xrust/blob/master/exercises/hello-world/GETTING_STARTED.md#understanding-test-failures - could you add that to this new test as well?

Thank you for your contribution!

@IanWhitney
Copy link
Contributor

I know that in the canonical tests we've pulled out words that couldn't be valid scrabble words. I'm guessing the same argument will apply here.

Before we update the Rust tests, I think the change should happen in the canonical tests.

@juleskers
Copy link
Contributor Author

Thanks for the feedback!
I'll make sure to add an #[ignore].

As for the canonical tests, that makes a lot of sense, I wasn't aware there was so much coordination going on between tracks!

As for the alphabet, you are correct that it's not a scrabble word. I would argue that the alphabet itself is the clearest test-case to prove that the entire alphabet is present.
If words are a hard requirement, I could try "the quick brown fox jumps over the lazy dog" (more a sentence than a word, I admit, but maybe in 8 asserts?)

As I mentioned in the other Pull-request, I won't be the fastest to reply, as I do this in my lost minutes on the train.

@petertseng
Copy link
Member

It did get added in x-common - don't forget to add the ignore here.

@juleskers
Copy link
Contributor Author

juleskers commented Jan 30, 2017

ignore added! I had it lying around already, but didn't have time/internet to push it.

@petertseng petertseng merged commit 6bfaeeb into exercism:master Jan 31, 2017
@petertseng
Copy link
Member

Thanks!

(I squashed the two commits before merging since these two commits make the most sense as a single one)

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.

3 participants