Clarify status and message in test runner spec
#175
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Consider the situation of: - The user submits a solution in a compiled language - The user's solution compiles without error - The first test passes, but the next test "produces an error" (for example, it raises an exception). In this case, the correct top-level status is `fail`, with one test-level status of `pass`, and one test-level status of `error`. But the previous wording could imply that a top-level status of `fail` is only correct if at least one test has the status `fail`.
The previous wording could imply that a per-test `message` property can only be present when the test `status` is `fail`.
To me, the previous wording implied that a top-level `status` of `error` is valid when at least one test "errors". But actually, if at least one test was "executed", the top-level status should be `pass` or `fail`.
ee7
commented
Jul 7, 2021
| @@ -119,15 +119,15 @@ The following per-test statuses are valid: | |||
|
|
|||
| - `pass`: The test passed | |||
| - `fail`: The test failed | |||
| - `error`: The test errored | |||
| - `error`: The test errored - that is, it did not return a value | |||
There was a problem hiding this comment.
I'm not happy with this, but I think we can do better than just "The test errored".
There was a problem hiding this comment.
Yeah this is good enough I feel
Co-authored-by: Erik Schierboom <erik_schierboom@hotmail.com>
Co-authored-by: Erik Schierboom <erik_schierboom@hotmail.com>
ee7
commented
Jul 22, 2021
ErikSchierboom
approved these changes
Jul 22, 2021
Co-authored-by: ee7 <45465154+ee7@users.noreply.github.com>
|
@ee7 Happy about the current version? |
|
@ErikSchierboom Yeah. Good enough for now, I think. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Reproducing the commit messages:
Clarify top-level
failin test runner specConsider the situation of:
example, it raises an exception).
In this case, the correct top-level status is
fail, with onetest-level status of
pass, and one test-level status oferror. Butthe previous wording could imply that a top-level status of
failis only correct if at least one test has the status
fail.Clarify per-test
messagein test runner specThe previous wording could imply that a per-test
messagepropertycan only be present when the test
statusisfail.Clarify top-level
errorin test runner specTo me, the previous wording implied that a top-level
statusoferroris valid when at least one test "errors". But actually, if at least one
test was "executed", the top-level status should be
passorfail.Try to clarify test-level
errorin test runner spec(Difficult).
I'm making this PR because in exercism/nim-test-runner#87 (comment) it was unclear to me what the expected
results.jsonis when a test errors.I wanted to incorporate Erik's clarification for a top-level
status:and for a test-level
status:because I think the current docs don't convey this.
I think
failvserrorcould be clearer elsewhere ininterface.mdtoo (I'd appreciate if someone could read through the fullinterface.mdcontents with this PR and help out), but I'll set this PR as non-draft in case we prefer to follow-up later.