-
-
Notifications
You must be signed in to change notification settings - Fork 654
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
improved output for simple_cipher test #2288
improved output for simple_cipher test #2288
Conversation
- made tests to output all failed data examples instead of one only - changed Encode test error log to contain an original string instead of the one with cleared extra symbols
Dear iAutomatorThank you for contributing to the Go track on Exercism! 💙
Dear Reviewer/Maintainer
Automated comment created by PR Commenter 🤖. |
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.
I agree that we should show the output for the other failed tests, but let me suggest another way to do this.
Right now we are not using subtests for this exercise, and subtests can have calls to t.Fatalf
in them, but still allow the other tests to run. We are in the process of converting the tests on the track to subtests, you can read more about this here: #2098
So my suggestion would be to convert the tests for this exercise into subtests, and keep the calls to t.Fatalf
inside the subtests. This way we fix 2 issues at once :)
When doing subtests, the call to t.Run
expects a "test name" as the first argument. Right now it seems we don't have descriptions attached to each test case. My suggestion would be to add a description field to the prepped
and cipherTest
structs and grab the descriptions for the tests from canonical-data.json. If there's a test that we have that is not present in that json file, feel free to write a small description for the test. If there are tests in the json file that we don't currently have, ignore those and we'll add them at a later date.
Let me know if you would you like to implement subtests for the tests of this exercise. I know this is a bigger ask than what you probably intended originally, so I totally get if you don't want to do this. But I truly believe this is the right way to fix this issue. If you make these changes, I'll make sure to size the PR accordingly so you get a little bit more rep for the extra work.
applied subtests so now output is as follows
|
additionally, dedicated separate tests for checking wrong cipher keys:
|
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.
Thanks for this!
Fixes
Before
Now
Before
Now
Related issue: #2098