generators: convert generators to use auto extract#567
generators: convert generators to use auto extract#567Insti merged 61 commits intoexercism:masterfrom
Conversation
hilary
force-pushed
the
generate_problem_name_cases
branch
2 times, most recently
from
5dc1462 to
db28d13
Compare
hilary
changed the title
| "phrase = '#{input}'", | ||
| " result = Pangram.pangram?(phrase)", | ||
| " #{assertion} result, \"#{message}\"" | ||
| " #{assert} result, \"#{message}\"" |
There was a problem hiding this comment.
Can do message.inspect here and get rid of the \"s
There was a problem hiding this comment.
Doesn't work here (at least straight substitution doesn't). The interpolation in message would also need adjusting.
| def test_bookkeeping | ||
| skip | ||
| assert_equal 2, BookKeeping::VERSION | ||
| assert_equal 3, BookKeeping::VERSION |
There was a problem hiding this comment.
Not sure the changes here warrant a version bump.
There was a problem hiding this comment.
It added a test, hence the version bump.
| def test_bookkeeping | ||
| skip | ||
| assert_equal 2, BookKeeping::VERSION | ||
| assert_equal 3, BookKeeping::VERSION |
There was a problem hiding this comment.
There are changed tests. Shouldn't that mean the version is updated?
There was a problem hiding this comment.
Which tests changed? I can't find any that have changed input or expected values.
The commit that does the version update should just include the bits of the file that caused that version upgrade.
These are being hidden by the rest of the noise in the commit due to formatting changes. (which don't require a version bump.)
There was a problem hiding this comment.
Ah, you are right. In this case, just the names changed.
| detector = Anagram.new('master') | ||
| anagrams = detector.match(["stream", "pigeon", "maters"]) | ||
| assert_equal ["maters", "stream"], anagrams.sort | ||
| assert_equal ["maters", "stream"], Anagram.new('master').match(["stream", "pigeon", "maters"]).sort |
There was a problem hiding this comment.
I strongly prefer the previous 3-line version of all these tests.
There was a problem hiding this comment.
Interesting. I strongly prefer the one line version. With the three line version, I have to mentally assemble the test; with the one line version, it is all right there.
I go to multiple lines on tests that are too complex to mentally wrangle by reading a single line (like the tests I wrote for Generator::CaseValues. But if I can do it in one or two lines, I prefer it.
How would you feel about a two line version?
anagrams = Anagram.new('master').match(["stream", "pigeon", "maters"])
assert_equal ["maters", "stream"], anagrams.sort
There was a problem hiding this comment.
It is worse than both the 3 and 1 line versions.
There was a problem hiding this comment.
ROFL!
The usual 3 line pattern for tests is
expected = ...
actual = ...
assert_equal expected, actual
These 3 liners cause cognitive friction for me because they are 3 liners that violate that pattern.
There was a problem hiding this comment.
4 would probably be best since you can use another line for expected
The main issue here is that we want to prioritize educational value over expert comprehension.
1 line:
assert_equal ["gallery", "largely", "regally"], Anagram.new('allergy').match(["gallery", "ballerina", "regally", "clergy", "largely", "leading"]).sortAt my day job I would be fine with the 1 line version as everyone knows the test framework and can maintain a consistent test style, and we are not trying to work out how to implement a solution by looking at the tests.
On Exercism I want to make sure that things are clear to people who may not be as familiar with Minitest and even Ruby.
Multi line:
detector = Anagram.new('allergy')
anagrams = detector.match(["gallery", "ballerina", "regally", "clergy", "largely", "leading"])
expected = ["gallery", "largely", "regally"]
assert_equal expected, anagrams.sortThe multi line version uses variable naming to reveals the intention of the individual elements of the one line assertion. Note that the values used for detector,anagrams and expected are all vary by test.
This provides the information one needs to derive the code to pass the test in a clear and consistent manner.
There was a problem hiding this comment.
Makes sense to me! I'll change it.
hilary
force-pushed
the
generate_problem_name_cases
branch
from
eca7663 to
ee75656
Compare
Just load the alpha_cases file direcly, we know where it is.
test/fixtures/metadata/exercises/alpha/canonical-data.json We are the only user of this fixture data.
Renamed 'gamma' to 'complex'.
Add test that template_values loads problem cases Remove commented out code. Make everything that loads AlphaCase(s) clean up after itself. Undefine AlphaCase(s) during teardown. Remove redundant setup. Check for both classes. Add tests for class based test generation squash Remove TODO comment
all generators converted to use auto extraction in exercism#567
removed tests that are now redundant
hilary
force-pushed
the
generate_problem_name_cases
branch
from
ee75656 to
c56bd33
Compare
|
Big thanks @hilary for all your work on this! ❤️ ⭐ 👍 |
2 participants
Motivation and Context
I converted all generators to use auto extraction (#568) so we don't need to maintain support for the old
procextraction technique.Description
(Commits 19d34af and e839613 are part of #568)
How Has This Been Tested?
In all cases,
rake testpasses.In all cases, regenerating the tests produces either the same tests (when the data has not changed) or the desired tests (for updated data)
Types of changes
Updates and fixes
References and Closures
Checklist:
My change requires a change to the documentationI have updated the documentation accordingly.I have added tests to cover my changes.