-
-
Notifications
You must be signed in to change notification settings - Fork 216
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
tests: update say exercise #903
Conversation
Hello. Thanks for opening a PR on Exercism 🙂 We ask that all changes to Exercism are discussed on our Community Forum before being opened on GitHub. To enforce this, we automatically close all PRs that are submitted. That doesn't mean your PR is rejected but that we want the initial discussion about it to happen on our forum where a wide range of key contributors across the Exercism ecosystem can weigh in. You can use this link to copy this into a new topic on the forum. If we decide the PR is appropriate, we'll reopen it and continue with it, so please don't delete your local branch. If you're interested in learning more about this auto-responder, please read this blog post. Note: If this PR has been pre-approved, please link back to this PR on the forum thread and a maintainer or staff member will reopen it. |
This PR touches files which potentially affect the outcome of the tests of an exercise. This will cause all students' solutions to affected exercises to be re-tested. If this PR does not affect the result of the test (or, for example, adds an edge case that is not worth rerunning all tests for), please add the following to the merge-commit message which will stops student's tests from re-running. Please copy-paste to avoid typos.
For more information, refer to the documentation. If you are unsure whether to add the message or not, please ping |
@vaeng everything should be fixed now. A couple of questions:
|
Hello @Elahi-cs, Thanks for your patience. You did the right thing to ask in the forum and also a nudge to look at this would have been fine. I think your change is good and welcome, I wanted to ask the other maintainers for feedback before going further, but I haven't had the time yet. As a result of my late reply, this September or will become tagged for hacktoberfest. I will not be able to check your code until Tuesday, but maybe @siebenschlaefer has some capacity this weekend. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes look mostly fine, but the two cases giving an error in CI need to be fixed.
To run exactly what happens in CI, you probably won't get around using something such as cmake .
# run all tests
cmake --build .
# alternatively, run just a single test:
cmake --build . -- test_say |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That looks good to me, I don't see any reason not to merge this now.
@ahans remarks seem to be addressed.
This PR is the result of running this command:
using the test creation helper tool linked in the wiki, with some modifications to the code necessary for the tool to work.