-
-
Notifications
You must be signed in to change notification settings - Fork 72
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
Add retry on failure support to JunitReporter #293
base: main
Are you sure you want to change the base?
Changes from all commits
1ffefd4
31f958b
6641dfb
4174b06
49b4559
d0d6360
fe00561
db9e55a
2fbda55
8d420db
de5f561
5308f56
c8fcf82
3c65355
f9bb063
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,12 +25,16 @@ package final class JunitReporter { | |
|
||
if FailingTestCaptureGroup.regex.match(string: line) { | ||
guard let testCase = generateFailingTest(line: line) else { return } | ||
// reduce failing retrys, if any | ||
components.removePreviousFailingTestsAfter(testCase) | ||
components.append(.failingTest(testCase)) | ||
} else if RestartingTestCaptureGroup.regex.match(string: line) { | ||
guard let testCase = generateRestartingTest(line: line) else { return } | ||
components.append(.failingTest(testCase)) | ||
} else if TestCasePassedCaptureGroup.regex.match(string: line) { | ||
guard let testCase = generatePassingTest(line: line) else { return } | ||
// filter out failing retrys, if any | ||
components.removePreviousFailingTestsAfter(testCase) | ||
Comment on lines
34
to
+37
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point, pushed some changes, thanks! |
||
components.append(.testCasePassed(testCase)) | ||
} else if TestCaseSkippedCaptureGroup.regex.match(string: line) { | ||
guard let testCase = generateSkippedTest(line: line) else { return } | ||
|
@@ -166,6 +170,32 @@ private enum JunitComponent { | |
case skippedTest(TestCase) | ||
} | ||
|
||
private extension JunitComponent { | ||
var testCase: TestCase? { | ||
switch self { | ||
case .testSuiteStart: nil | ||
case let .failingTest(testCase), let .testCasePassed(testCase), let .skippedTest(testCase): | ||
testCase | ||
} | ||
} | ||
} | ||
|
||
private extension [JunitComponent] { | ||
mutating func removePreviousFailingTestsAfter(_ testCase: TestCase) { | ||
// base case, empty array or last is not the given TestCase | ||
guard let previousTestCase = last?.testCase, | ||
previousTestCase.failure != nil, | ||
testCase.classname == previousTestCase.classname, | ||
testCase.name == previousTestCase.name | ||
else { | ||
return | ||
} | ||
removeLast() | ||
// keep removing | ||
removePreviousFailingTestsAfter(testCase) | ||
} | ||
} | ||
|
||
private struct Testsuites: Encodable, DynamicNodeEncoding { | ||
var name: String? | ||
var testsuites: [Testsuite] = [] | ||
|
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'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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?