state-of-tic-tac-toe: tweak error nessage and add test case
#2012
Conversation
jiegillet
added
new test case idea
x:action/improve
jiegillet
force-pushed
the
jie-state-of-tic-tac-toe
branch
from
a7623a5 to
9b08e67
Compare
|
I guess so, I didn't think so either, but the CI didn't let me change just the error... |
|
The CI script doesn't really consider errors explicitly, it's mostly a side effect. @ErikSchierboom Should error messages be immutable too? To me they seem like descriptions. |
|
I think the question here would be what the error text is. Is it like the description, an error that explains why it should be an error to the maintainers so that they interpret it in a way that makes sense in their language, or is it the actual error message that the student would have to return? To me it seems like it should be the former because it's impossible to write the latter in a language-agnostic way. If it's the former, it should be mutable. |
|
That's true, depending on how you want to implement the exercise, you might not return the error as this exact string, although I do feel that many track probably would do that, since returning an error with a string is a pretty standard. I usually choose to do that in Elixir/Elm. Since I can ask students to return strings, why wouldn't I use the one provided to me here? Another point is that I would want the maintainers to see that an error message has been changed. Does configlet inform people of such a change if the test case has not been reimplemented? If it doesn't, then it's a point for immutable, but I can't remember if it does... |
No, we don't show these changes. We've had discussions on the error text before, but we hadn't yet reached a satisfactory conclusion I think. I think I'm still leaning towards reimplementation, as some tracks will be using the error description to influence what code is being generated. |
|
I'm not sure I understand. |
| }, | ||
| { | ||
| "uuid": "4801cda2-f5b7-4c36-8317-3cdd167ac22c", | ||
| "description": "Invalid board", |
There was a problem hiding this comment.
I just noticed that every test case for invalid board has the exact same description, which will definitely trip up test generators. It also not very descriptive. Maybe we could update the descriptions of the invalid board test cases to include the error?
There was a problem hiding this comment.
I just noticed that every test case for invalid board has the exact same description, which will definitely trip up test generators.
See #1991 (comment)
There was a problem hiding this comment.
It's not just the invalid board, most descriptions repeat 3 times...
On the Elixir implementation I added (1) (2) (3)... To all descriptions. It was a bit tedious.
Shall I do the same, or better descriptions?
There was a problem hiding this comment.
Shall I do the same, or better descriptions?
Adding the errors to the descriptions here seems reasonable but I'd leave the other descriptions for a different PR.
In the last test, the board is
and it's clear that
Xwon andOkept playing, but for this board:it's impossible to determine who won "first".
And really, it doesn't matter who won, what matters is that this is an impossible board because the players kept going after the game was won, so I'm proposing to tweak the error message to reflect that and add one more test case to justify the change.
Credits to @angelikatyborska for finding this incompleteness issue.