-
-
Notifications
You must be signed in to change notification settings - Fork 59
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
Spec generator - Respect tests.toml #524
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 |
-- LuaFormatter off | ||
it('check that groups of four are created properly even when there are more groups of three than groups of five', function() | ||
it('check that groups of four are created properly even when there are more groups of three than groups of five', | ||
function() | ||
local basket = { 1, 1, 1, 1, 1, 1, 2, 2, 2, 2, 2, 2, 3, 3, 3, 3, 3, 3, 4, 4, 5, 5 } | ||
assert.are.same(14560, book_store.total(basket)) | ||
end) | ||
-- LuaFormatter on |
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 re-generated all specs to make sure that we hadn't inadvertently included tests that shouldn't have been included. When I did this it reverted the change that was made in #513 to fix an issue with the test runner that has since been resolved.
it('finished game where x won via middle row victory', function() | ||
local board = { | ||
'O O', -- | ||
'XXX', -- | ||
' O ' -- | ||
} | ||
assert.are.same('win', state_of_tic_tac_toe.gamestate(board)) | ||
end) |
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 think this is the test that was manually removed. It must have slipped back in the next time the spec generator was run. We should be protected against this now.
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.
That was a different test case I accidentally neglected to remove.
The case I removed:
description = "Invalid boards -> Invalid board"
include = false
was essentially the same as
description = "Invalid boards -> Invalid board: X won and O kept playing"
but with a different error string. (I stopped checking the error strings, anticipating the test case might get regenerated.)
In general, does this track have a preference for or against checking error strings?
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 general, does this track have a preference for or against checking error strings?
At least for recent tests I've been checking the strings. I don't feel strongly one way or the other.
We can safely merge with |
In #523, @keiravillekode mentioned that the
reimplements
was handled by manually updating the generated spec. This updates the spec generator so that only tests included viatests.toml
are included in the generated spec.