-
-
Notifications
You must be signed in to change notification settings - Fork 529
Add templates for exercises batch 7 #1787
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
base: main
Are you sure you want to change the base?
Conversation
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 |
a174f7b
to
ff20135
Compare
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.
Approving, as these are "good", but we might consider the templates being more cookie cutter.
|
||
class PalindromesTest < Minitest::Test | ||
def test_smallest_palindrome_from_single_digit_factors | ||
def test_find_the_smallest_palindrome_from_single_digit_factors |
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.
Not really a fan of the phrase "test find the" but "test for the" makes sense to me.
sentence = '' | ||
result = Pangram.pangram?(sentence) | ||
refute result, "Expected false, got: #{result.inspect}. #{sentence.inspect} is NOT a pangram" | ||
refute result, "Expected false, got: #{result.inspect}. #{sentence.inspect} is not a pangram" |
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.
Have we slipped from using "expected, actual" as a pattern? The result, I would suspect, would be resolution of the tests, rather than an evaluation of what is actually being under test.
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 am not sure what you mean. Could you give an example when this happen?
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.
In history? I think the old templating system, I used expected, actual was used for that.
Structurally, or as a pattern, I think it is helpful to have:
class TriangleTest < Minitest::Test
<% json["cases"].each do |cases| %>
def test_<%= underscore(cases["description"]) %>
<%= skip? %>
actual = Triangle.new(<%= cases["input"]["count"] %>).rows
expected = <%= cases["expected"] %>
assert_equal expected, actual, 'Helpful message if appropriate"
end
<% end %>
Which in every assertion line being something like assert expected, actual, message
, and the details of what the expected and actual being on their own lines, which can make reading the tests easier as well.
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.
Okay I I think I understand. And sorry for taking sometime for reply. I have had quite a bit this week so I only have the weekend over to open source work.
A big part of templating is just that we can in aftermath make changes really quickly, like 1 line change or something can do it for an entire test file. My goal with the templating is to make the styling the same as the old one mostly as a proof on concept that the generator can handle it. In some cases have I changed the test cases in either dissucused style changes or to make the exercise in line with problem spec.
I have no problem start implementing these style changes now but on the other hand there is quite a few already merged in templates so as I see it we also have to go back and change those as well.
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.
Right on. I did consider not saying anything at all... and it could be an "after the fact" change, since it is definitely pattern work, and should not affect test functionality.
If I remember correctly, I was at least co-author of the original templating system that was here before. And also knew that students are encouraged to read the tests (at least for the practice exercises) as there were no concept exercises back then at all.
If you want to see where we can keep the simplistic assertion lines by making expected, actual be the pattern, that would be appreciated I think even further than only myself. But I would also be tickled pink, if that were possible.
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.
This could easily be a single assertion, though. Store the expected results in a collection expected values and assert that the collection of actual values are identical to the collection of expected values.
I have never really loved that test either. ;)
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.
Yeah but like should we also do a singel assertion in palindromes? It is here I think a distiction in how we handle these kinds of cases have to be made (and it could be a mix of those two it doesnt have to be one or the other).
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.
Can you link to what you are looking at? I went to the palindromes Line 5 to see what I could see.
This is how it is:
def test_smallest_palindrome_from_single_digit_factors
palindromes = Palindromes.new(max_factor: 9)
palindromes.generate
smallest = palindromes.smallest
assert_equal 1, smallest.value
assert_equal [[1, 1]], smallest.factors
end
This is how it may be (untested, here for demonstration and not refactored to "base":
def test_smallest_palindrome_from_single_digit_factors
palindromes = Palindromes.new(max_factor: 9).generate # if generate returns the instance
actual = palindromes.smallest.value, palindromes.smallest.factors
expected = 1, [1, 1]
assert_equal expected, actual
end
This may be improved further, not looking at the exercise in full. But kind of pattern can be useful for us overall. It also presents a certain pattern of familiarity for the student, as they know what to look for while reading the test files, and how to decipher them a bit more easily, I think.
This generator, yeah, it is for our benefit, but everything exposed to the student, the result that comes from the generator, should be for their benefit.
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.
Alright I made a new commit with the changes we have disscussed let me know if it was this you had in mind. Since what I was thinking about was that the palindrome products case with assert_includes
might be a bit confusing but it might actually work.
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 modified your message for what I would encourage for readability for the reader. It gets the reader to the points that you have in mind (hopefully) to look at without having to spend time working to interpret what is actually referred to.
Not a criticism, but you likely have what is in mind, the reader needs to interpret this to understand, and I hope that the changes reflect what your thoughts were.
<% json["cases"].each do |cases| %> | ||
def test_<%= underscore(cases["description"]) %> | ||
<%= skip? %> | ||
meetup = Meetup.new(<%= cases["input"]["month"] %>, <%= cases["input"]["year"] %>).day(:<%= cases["input"]["dayofweek"].downcase %>, :<%= cases["input"]["week"] %>) |
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.
Conceptually, and as a pattern, the tests are assert expected, actual
and we can use these names. If we are careful with this, a high percentage of template portions will follow this pattern, the only difference being what is referenced by actual
as to what is called, while the expected is always what we expect.
a1ebd9b
to
6177eba
Compare
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.
Definitely looking better, not tested locally.
assert_equal 9, largest.value | ||
assert_includes [[[1, 9], [3, 3]], [[3, 3], [1, 9]]], largest.factors | ||
actual = palindromes.largest.value, palindromes.largest.factors | ||
expected = [[9, [[1, 9], [3, 3]]], [9, [[3, 3], [1, 9]]]] |
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.
Likely one extra pair of []
brackets... Gotta go back and look at how this is created. But also, perhaps not really problematic.
Just knowing that the assignment itself will be evaluated as an Array is good information to have in mind as a Ruby developer though.
We should avoid nesting to the extent that we can though, as it can be confusing as a learner. (It goes the other way, though, if you are unaware.)
Includea: