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

rna-transcription: Update tests to expect ArgumentError if invalid input is supplied #848

Merged
merged 2 commits into from
Aug 27, 2018
Merged

rna-transcription: Update tests to expect ArgumentError if invalid input is supplied #848

merged 2 commits into from
Aug 27, 2018

Conversation

tylerferraro
Copy link
Contributor

The Ruby rna transcription challenge has several tests that expect an empty string to be returned if invalid input is supplied. I feel it's better to expect an error/exception to be thrown as is done in other language implementations such as Python. It will help students build good habits around class and method design.

@Insti
Copy link
Contributor

Insti commented Aug 24, 2018

Thanks for doing this @tylerferraro ❤️

The error case tests have been removed from the canonical data

So this won't be an issue any more once the tests are regenerated.

Copy link
Contributor

@Insti Insti left a comment

Choose a reason for hiding this comment

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

Doing the generator changes was the right thing to do. 👍

But in this case if you can remove the generator changes, I can merge the test file changes so they can be included until the tests get regenerated from the updated canonical-data.json

@Insti Insti changed the title Update rna transcription tests to expect ArgumentError if invalid input is supplied rna-transcription: Update tests to expect ArgumentError if invalid input is supplied Aug 24, 2018
@tylerferraro
Copy link
Contributor Author

@Insti Just to make sure I understand correctly. By removing the tests from the canonical data, when the tests are generated next time, these error test won't exist at all, correct? Wouldn't it benefit the student to think about edge cases such as a string with invalid characters?

@Insti
Copy link
Contributor

Insti commented Aug 24, 2018

By removing the tests from the canonical data, when the tests are generated next time, these error test won't exist at all. correct?

This is correct.

Wouldn't it benefit the student to think about edge cases such as a string with invalid characters?

Probably. But trying to do/test too much at the same time is also an issue.

There should be a dedicated string validation exercise for thinking about string validations.

If you see string validation issues in solutions you should mention it as part of the mentoring process but I don't think it should be part of the fixed test cases.

@Insti
Copy link
Contributor

Insti commented Aug 24, 2018

See also: exercism/problem-specifications#902

@tylerferraro
Copy link
Contributor Author

Now that this is approved, what's the process for merging?

@Insti
Copy link
Contributor

Insti commented Aug 24, 2018

I usually wait a while to see if any of the other maintainers want to comment and/or agree and hit the merge button. If nothing happens, I merge it myself.

@Insti Insti merged commit d78cf7f into exercism:master Aug 27, 2018
@Insti
Copy link
Contributor

Insti commented Aug 27, 2018

Thanks @tylerferraro ❤️

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.

2 participants