accumulate: expand tests to require general solutions#645
accumulate: expand tests to require general solutions#645coriolinus merged 1 commit intoexercism:masterfrom
Conversation
| "difficulty": 4, | ||
| "topics": [ | ||
| "function_pointer" | ||
| "generics", "higher_order_functions", "move_semantics" |
There was a problem hiding this comment.
Because you've changed config.json, you'll need to rerun configlet fmt in order for Travis to succeed.
coriolinus
left a comment
coriolinus
left a comment
There was a problem hiding this comment.
Thanks for doing this, @Emerentius! There are a few points which are preventing Travis from passing this, but in general, I like the way you've rewritten the tests.
| @@ -1,4 +1,9 @@ | |||
| // Note: The tests for this exercise are not #[ignore]d, | |||
| // but commented out with a multi-line comment | |||
| /* this is a multi-line style comment */ | |||
There was a problem hiding this comment.
I'm not a fan of commenting out the tests instead of using #[ignore], and neither is Travis. Beyond aesthetic reasons, using comments instead of attributes defeats common idioms such as cargo test -- --ignored. This exercise already has .meta/ALLOWED_TO_NOT_COMPILE set, which should prevent Travis from complaining about the compile failures.
There was a problem hiding this comment.
Ignored tests are still type checked. If I didn't comment them out, a student would be confronted with compile errors until they already had the final, fully general function signature.
There was a problem hiding this comment.
Yes, that's true. It's also more or less unavoidable when the point of the exercise is to figure out how to write a generalized generic function signature.
Reading the compiler errors and fixing them is an essential skill for anyone studying a language with a compiler. We try to arrange things to minimize the compiler errors a student is initially presented with, but IMO it's a good thing that there are a few exercises for which they just have to figure out what's going on based on what the compiler tells them.
There was a problem hiding this comment.
Right, but need it be >100 lines of compiler errors at once? The compiler doesn't stop at the first function. You can't even write a minimal map implementation for i32 -> i32 before having written fn map<T, U, F: FnMut(T) -> U>(input: Vec<T>, mut function: F)
It's a bit overwhelming on the command line.
If we don't comment the tests out, the student needs to have that idea on their own or advancing can become very frustrating.
It's better if you use an editor that shows you the compile error sources directly in the source, but even that is probably very tedious if you aren't already familiar with generics and the closure traits. I may be wrong there, I'm too far removed from the newbie experience by now.
Side note on in-editor errors: I'm not sure why, but this exercise drives RLS crazy. It's permanently pegged at 100% in VS Code.
I just try-solved it again with all tests active from the start and the compiler told me at some point that it needed Debug bounds. That was a misdiagnosis, the correct solution makes them unnecessary. I'll add a final test with a struct that has only the bare minimum of traits.
There was a problem hiding this comment.
need it be >100 lines of compiler errors at once?
Compilers do that sometimes. It may seem overwhelming at first, but with practice, one learns to just take them one at a time. Getting that comfort level requires exposure to the phenomenon. What's worse: hitting that for the first time here, in a mentored environment, or hitting it at some point in the future in their own code?
I wouldn't object to some text in the hints file that if the student finds the number of errors overwhelming, they can try temporarily commenting out later tests to reduce the scope of what needs to be implemented. I just don't think we should block-comment large sections ourselves.
There was a problem hiding this comment.
It may seem overwhelming at first, but with practice, one learns to just take them one at a time. Getting that comfort level requires exposure to the phenomenon.
That's true to some extent, but I'm not sure how valuable it really is as a teaching moment as opposed to just annoying. Students are also often expected on exercism to reverse engineer the interface and behaviour from the tests. I personally am mostly annoyed with that. It's not difficult, but tedious.
I can add a note to the hint and #[ignore] the tests again.
If I'm reading the error right, travis enforces that only 1 test is not ignored, is that right? For this exercise that's barely any different to having them activated by default. As soon as the compile errors are gone and the first two tests pass, all the others probably do, too.
There was a problem hiding this comment.
I actually completely agree that it's annoying to have to reverse-engineer the interface and behavior from the tests. We finally got to the point where we can programmatically verify that all exercises have a non-empty src/lib.rs file literally yesterday; it just took a long time to cover all the existing exercises.
Still, I think we're in pretty good shape as far as that goes. As of right now, there are 88 exercises (fd -td -d1 . exercises | wc -l), of which only 7 don't compile when the student first downloads them (fd -H ALLOWED_TO_NOT_COMPILE exercises | wc -l).
You're right that travis verifies that exactly 1 test is not ignored. By convention, the non-ignored test should be the first. Even if it barely makes a difference, it's better to avoid using magic travis-appeasing files when it is possible to just do the right thing.
|
|
||
| /// What should the type of _function be? | ||
| pub fn map(input: Vec<i32>, _function: ???) -> Vec<i32> { | ||
| pub fn map(input: Vec<i32>, function: ???) -> Vec<i32> { |
There was a problem hiding this comment.
Eliminating the leading underscore from _function will cause the compiler to complain about an unused variable. Despite the fact that this exercise is allowed to not compile, I'd prefer to minimize the number of compiler errors the student gets when they first download the exercise.
There was a problem hiding this comment.
I'll add it back, although it barely changes anything what with all the other compiler errors.
|
Incorporated the feedback and kept it separate for easier review. I'll squash them when the time to merge comes. |
coriolinus
left a comment
coriolinus
left a comment
There was a problem hiding this comment.
Thanks for doing this work @Emerentius! I'm now going to leave this for a few days to give other maintainers a chance to comment as desired, but if there are no objections, I'll plan to merge this not later than Wednesday the 5th. If on the 6th I have not yet done so, please feel free to ping me a reminder.
The previous tests let students pass with hard-coded input and output types and needlessly specific trait bounds.
Emerentius
force-pushed
the
ratchet_it_up
branch
from
ad6c484 to
6f94774
Compare
2 participants
Implements the test scheme I proposed in #629 which enforces that solutions are generally applicable and minimally restrictive.
Because ignored tests still cause compile failures, I had to comment them out entirely. I've used a multi-line comment so that including more tests can be done by just moving 2 characters. This is similar to how the C++ track organizes their tests and it's also quite fitting here, because it further encourages going through the tests in order.
An alternative would be to add a series of cargo features and conditionally compile the tests but that would be more work for students and a new concept to learn.