-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Fix test reporting #4379
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
Fix test reporting #4379
Conversation
When running the tests, we were reporting the number of errors encountered instead of the number of failed tests. For neg tests, errors are not test failures
|
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.
The original change made sense, in fact this errorCount
field was a misfeature, so I went further and removed it. @allanrenucci can you review that?
if (test.errorCount == 0) "" | ||
else s"\n - encountered ${test.errorCount} error(s)" | ||
if (test.failureCount == 0) "" | ||
else s"\n - encountered ${test.failureCount} error(s)" |
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 would change error(s)
for failure(s)
bf99300
to
60dd039
Compare
@Blaisorblade I am not sure about the second part of this PR. I think I would suggest to open a new PR with your changes |
That still works, and it's still visible in the meta test, in the output line
The code you talk about is not affected — 565a2e5 changes some other behavior, the rest is purely meaning-preserving dead code elimination. The behavior you describe involves another field named FWIW, I don't have a problem with splitting the other commits if there is plausible reason at all, this just isn't one. |
The reason is that it's safe to merge the first commit and I'd like to experiment with the other commits to convince myself that it is indeed dead code |
29f4760
to
df78c3c
Compare
94879b9
to
df78c3c
Compare
The current PR was tested in http://dotty-ci.epfl.ch/lampepfl/dotty/4656, so merging. |
When running the tests, we were reporting the number of errors
encountered instead of the number of failed tests. For neg tests, errors
are not test failures