ocamltest: also announce system errors as test errors#1634
Merged
gasche merged 1 commit intoocaml:trunkfrom Feb 26, 2018
Merged
ocamltest: also announce system errors as test errors#1634gasche merged 1 commit intoocaml:trunkfrom
gasche merged 1 commit intoocaml:trunkfrom
Conversation
Contributor
|
While we're on that subject, I've been wondering why the "ocamltests" file was required at all. Don't all ocamltest tests require a "(* TEST ... *)" comment at the beginning of the file? |
2ab26ff to
37dca89
Compare
Contributor
|
Thomas Refis (2018/02/26 10:28 +0000):
While we're on that subject, I've been wondering why the "ocamltests"
file was required at all. Don't all ocamltest tests require a "(* TEST
... *)" comment at the beginning of the file?
They do, but not all .ml files are tests and there may be tests that do
not end in .ml
Anyway I think this can be duscussed again after the migration when
there is only one kind of tests to handle.
|
Contributor
|
If there is a serious problem such as the file not found, I don't think
this should count as a test failure.
I mean, in my opinion, such failures need to be distinguished from
regular test failures and htat's why I didn't print the message.
|
Member
Author
|
Not printing the message has a disadvantage: it makes it much harder for human users to find the source of the problem from the CI log output, as was demonstrated this morning. I see no advantage. (I'm not proposing to change whichever policy you have towards stderr or exit values, just to print more information on severe failure to play nice with the existing test harness. And the change is only a couple lines.) |
Contributor
|
Could we get it to show up in the "Unexpected errors" count rather than the failed tests count? |
Member
Author
|
Sure, it's a one-line change, will do. |
Contributor
|
Leo White (2018/02/26 03:36 -0800):
Could we get it to show up in the "Unexpected errors" count rather
than the failed tests count?
Yeah, I think that would be better!
|
37dca89 to
2a6872f
Compare
Member
Author
|
I changed to print |
Contributor
|
Gabriel Scherer (2018/02/26 13:25 +0000):
I changed to print `unexpected error` instead of `failed`, which has
the desired behavior at summarization-time.
Thanks.
Would some of you consider approving the change?
Can you please update the commit message to reflect this change?
You could even retitle the PR
|
2a6872f to
03e8d25
Compare
Member
Author
|
I changed the commit message and PR title. |
If you forget to commit a test file with your patch (but include it in ocamltests), ocamltest will fail (print a message to standard error and return a non-zero code), but until now it would not print the standard-format "testing ... => unexpected error" message which makes the failure visible to the summarize.awk script. This commit ensures that such early failures still result in an explicit unexpected-error message being shown in the standard way, in addition to stderr-level error reporting.
shindere
approved these changes
Feb 26, 2018
Member
Author
EmileTrotignon
pushed a commit
to EmileTrotignon/ocaml
that referenced
this pull request
Jan 12, 2024
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
If you forget to commit a test file with your patch (but include it
in ocamltests), ocamltest will fail (print a message to standard error
and return a non-zero code), but until now it would not print the
standard-format "testing ... => failed" message which makes the
failure visible to the summarize.awk script.
(See #1565 (comment))
This commit ensures that such early failures still result in an
explicit test-failed message being shown in the standard way, in
addition to stderr-level error reporting.