Add retry on failure support to JunitReporter #293
Conversation
Not only the previous one
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #293 +/- ##
==========================================
+ Coverage 86.85% 87.01% +0.15%
==========================================
Files 14 14
Lines 1651 1671 +20
==========================================
+ Hits 1434 1454 +20
Misses 217 217 ☔ View full report in Codecov by Sentry. |
| // reduce failing retrys, if any | ||
| components.removePreviousFailingTestsAfter(testCase) |
There was a problem hiding this comment.
I'm concerned about removing previous failed iterations. We can't guarantee that test iterations fail for the same reason, so consolidating them here would suppress potentially important information. Looking at the JUnit specification and conventions, I think the current logic is correct– iteration failures should be recorded individually.
There was a problem hiding this comment.
I don't know much about JUnit but afaik there is no built-in support for the retry-on-failure mechanism so I don't think JUnit is opinionated.
It is acceptable to agree that if one of the retries succeeds, the output should not contain a failure from such a test.
However, I get your point if all the retry fails for different reasons, but should they be reported more than once if it is one test? Should we fail twice because the error is different even if there is just one test failing? or should we make some decisions and grab one?
| } else if TestCasePassedCaptureGroup.regex.match(string: line) { | ||
| guard let testCase = generatePassingTest(line: line) else { return } | ||
| // filter out failing retrys, if any | ||
| components.removePreviousFailingTestsAfter(testCase) |
There was a problem hiding this comment.
If a developer enables the "Run tests repeatedly" option, and each iteration passes, this will only show as 1 success, right? I don't think that's the desired behavior, and I don't think the current proposed changes consider that scenario. It's not one covered by the existing unit tests, but it should be.
There was a problem hiding this comment.
Good point, pushed some changes, thanks!
| mutating func removePreviousFailingTestsAfter(_ testCase: TestCase) { | ||
| // base case, empty array or last is not the last passed test case | ||
| guard let previousTestCase = last?.testCase, | ||
| testCase.classname == previousTestCase.classname, | ||
| testCase.name == previousTestCase.name | ||
| else { | ||
| return | ||
| } |
There was a problem hiding this comment.
The method name says removePreviousFailing, but the guard doesn't actually consider if the last test is a failing one. It'd need to check that testCase.failure is non-nil.
There was a problem hiding this comment.
Indeed, this is required for Run tests repeatedly
Makefile
Outdated
| .PHONY: xcode | ||
| xcode: | ||
| open Package.swift |
There was a problem hiding this comment.
Nice! Can you include this change in a new PR? It'd be nice to get this change merged while we decide the path forward on this PR.
There was a problem hiding this comment.
Sure, no problem!
There was a problem hiding this comment.
Hey @nandodelauni. Thank you for your PR and patience! I've left some comments regarding concerns I have about merging this change.
From what I can tell, the current implementation seems to match the JUnit specifications and conventions. That said, I don't use JUnit, so I'm not familiar with many of its nuances or how other tools implement similar logic.
Could you help me better understand how you decided on this approach? Are there any resources or specifications you referenced that indicate how to handle iterations and retry failures?
Add it into a separate PR
🎩 What is the goal?
Avoid false positives when retry on failure is enabled and there is one retry that passes. Right now the test is reported as both success and failure.
🔨 How is it being implemented?
By filtering out test failures if a test case passes and the previous components have the same
classnameandclasswhich uniquely identifies a test.Closes #290