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

Fix all-your-base test. For great justice! #207

Merged
merged 2 commits into from
Sep 4, 2016

Conversation

rmunn
Copy link
Contributor

@rmunn rmunn commented Sep 3, 2016

Fixes #206 by allowing any number of sequential zeroes to represent 0, but considering an empty list of digits to still be an error. (We can fix that one later if necessary).

@rmunn
Copy link
Contributor Author

rmunn commented Sep 3, 2016

The AppVeyor build runs the tests, but the Travis build doesn't? Weird.

... Oh, I see. The Travis build is honoring the Ignore attribute, but for some reason the AppVeyor build is actually ignoring the Ignore attribute and running the marked-as-ignored tests anyway. And the tests are failing because I updated the tests, but I didn't update the example code.

I'd rather not update the example code until after I do the exercise myself, so that I come at it fresh. If someone else wants to update the example, that would be great. Otherwise, I'll get to it, but it might be a couple of days.

@ErikSchierboom
Copy link
Member

Odd that the Travis build succeeds, it should have the same behavior. Both build servers should indeed run all tests, including those marked as ignored. The rationale behind that is that we want to be sure that our example code passes all tests.

I can look into fixing the example code monday probably.

@ErikSchierboom
Copy link
Member

@rmunn I saw that you posted a correct implementation that works with the modified tests on exercism. Would you mind adding your correct implementation to this PR as the example implementation for this exercise?

This example passes the new unit tests.
@rmunn
Copy link
Contributor Author

rmunn commented Sep 4, 2016

@ErikSchierboom New example implementation added, all tests passing. PR should be ready to merge now.

@ErikSchierboom ErikSchierboom merged commit 2819389 into exercism:master Sep 4, 2016
@ErikSchierboom
Copy link
Member

Merged. Thanks! 🎉

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.

Tests for all-your-base look wrong
2 participants