If a test failure isn't found in the test case map, try to assign it to the suite matching a class that is #15023
Conversation
| @@ -183,6 +184,18 @@ public void testFailure(Description description, Throwable throwable) { | |||
| test.testFailure(throwable, now()); | |||
| } | |||
| } | |||
| else { | |||
| final Optional<Description> matchingSuite = testsMap.keySet().stream() | |||
There was a problem hiding this comment.
I think it's worth adding a comment indicating how this scenario occurs and what the motivation for the code is.
| final TestCaseNode newTest = new TestCaseNode(description, suite); | ||
| suite.addTestCase(newTest); | ||
| testsMap.put(description, newTest); | ||
| newTest.testFailure(throwable, now()); |
There was a problem hiding this comment.
TestNode differentiates between testFailure and dynamicTestFailure. I'm not familiar with this code, but would it not be better to do this as well?
Perhaps the code should be written so the logic to get a TestNode object (and update the map) comes at the top, and then the original code just operates on that?
There was a problem hiding this comment.
@c-mita for this particular kind of exception, at least, when I try replacing the call to testFailure with dynamicTestFailure, I don't get a full stack trace in the final XML, despite passing through the Throwable.
So I'm a little hesitant to reorder the logic, because I don't understand the circumstances under which a DynamicTestException will appear in the top part of the if statement.
There was a problem hiding this comment.
After a little more investigation, I believe that DynamicTestException is unused within Bazel.
It is used within Google however, so some care is needed. I'll need to investigate a bit more as changes here could have impacts beyond Bazel's test exection.
There was a problem hiding this comment.
That using dynamicTestFailure all the time surpresses full stack traces doesn't surprise me, because that appears to be part of its intent. Its origin lies in handling Javascript test suites (which is an area I have no expertise in).
But the code I had in mind was this:
if (throwable instanceof DynamicTestException) {
DynamicTestException dynamicFailure = (DynamicTestException) throwable;
newTest.dynamicTestFailure(dynamicFailure.getTest(), dynamicFailure.getCause(), now());
} else {
newTest.testFailure(throwable, now());
}
Essentially replicating what's done in the method above.
There was a problem hiding this comment.
Moved the logic into getTest() think that's what you meant originally.
There was a problem hiding this comment.
This means this new logic will run for more than just test failures; I don't know if this is a good thing or not.
|
Hello @gmishkin, Can you please resolve the above comments and send back for review. Thanks! |
|
@sgowroji I am just waiting for the outcome of the internal investigation in #15023 (comment) before proceeding further. I could start typing up a code comment for the first review comment above, but it'd simply be restating the pull request description, and I'd rather wait til the other comment is resolved in case it affects the patch or wording. |
sgowroji
added
awaiting-review
gmishkin
force-pushed
the
assign-unexpected-tests-to-matching-suite-2
branch
from
c8e90be
to
13ff0f1
Compare
|
@c-mita the tests that are failing don't look like they're related to these changes, please advise if you think there's something I need to take a closer look at. |
After cursory glance, I don't think your change is causing those failures. |
| return test; | ||
| } | ||
|
|
||
| final Optional<Description> matchingSuite = testsMap.keySet().stream() |
There was a problem hiding this comment.
This still needs a comment explaining why this is being done.
Especially considering that now a "getTest" method can result in internal state being changed.
hvadehra
added
awaiting-user-response
Mockito unnecessary stubbing detection (
org.mockito.exceptions.misusing.UnnecessaryStubbingException) fires off failures outside the context of a test case, but since the Description isn't found intestsMap, the details aren't written to the test.xml (however it still goes to stdout and the overall bazel test fails).This change attempts to look up a matching suite in the test map by its Class. If found, it creates a new test case node and marks it as failed. I tried using the
dynamicTestFailurethingy directly on the suite, and that would increase the failure count, but the failure details wouldn't be added to the XML.