-
Notifications
You must be signed in to change notification settings - Fork 41
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
Xcode reports #30
Xcode reports #30
Conversation
Awesome! 👏 |
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.
Thanks for this @ilyapuchka, this brings some ideas and shortcuts to getting better Xcode reporting without having to implement the runtime test creation.
While reviewing this I had some ideas on how to simplify it and created #32. Wondering what you think of the approach there.
I've posted some feedback I written prior to #32 for posterity.
Sources/Spectre/Reporters.swift
Outdated
var description:String { | ||
if isatty(STDOUT_FILENO) > 0 { | ||
if isatty(STDOUT_FILENO) > 0 && ANSI.isEnabled { |
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.
In Commander @AliSoftware has made the ANSI compatible terminal check a bit smarter, which I believe should solve the detection for Xcode. Could you see if this works for your case too?
Check out kylef/Commander@d08cd61#diff-ece760f5fa0538199b1374e79adce33cR43
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.
After some testing, this approach indeed works in Xcode. I've committed this into master: af4c327.
Sources/Spectre/Global.swift
Outdated
reporter = StandardReporter() | ||
#else | ||
reporter = XCTestCase.reporter |
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.
Could we please reverse this #if os
statement so it explicitly checks for tvOS, macOS and iOS. Otherwise this will likely break on other platforms like BSD and Windows (if they even work with Spectre, I do not have any reliable CI to test).
@@ -15,6 +15,6 @@ public func testFailure() { | |||
// We cannot trust fail inside fails tests. | |||
fatalError("Test failed") | |||
} | |||
} | |||
} |
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.
NIT: Trailing whitespace
Sources/Spectre/Global.swift
Outdated
} | ||
|
||
} | ||
|
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.
Trailing whitespace on empty lines. Could you please remove the spaces on empty lines? Not sure what editor you are using, but you should be able to configure your editor to prevent this happening in the future.
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 think Xcode still adds spaces on empty lines when indenting everything. I'll run swiftlint autocorrect to remove them.
Sources/Spectre/Context.swift
Outdated
@@ -34,6 +34,15 @@ class Context : ContextType, CaseType { | |||
var afters = [After]() | |||
|
|||
init(name: String, disabled: Bool = false, parent: Context? = nil) { | |||
var name = name |
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.
Here XCTest support is interfering and complicating unrelated parts of Spectre, I'd like like to see core models in Spectre left untouched and to keep such logic out of PODOs (Plain Old Data Objects). This could be rearchitected to be inside the reporter itself so the XCTest specific logic stays in XCTest specific code areas.
@kylef #32 looks nice, though my idea was to keep standard reporter output, I find Xcode output much less readable. But I didn't find a way to suppress it, so it still interferes with standard reporter output a bit. In your case when tests are running with |
# Conflicts: # Sources/Spectre/Reporters.swift
7021d55
to
331232e
Compare
331232e
to
2b520ec
Compare
I'd love for one of these solutions to be implemented! In my mind there are 3 main things that would be useful from an XCTest integration
|
I'm going to merge #32 for now, just to get something merged sooner so we can benefit from it. However I'm not going to close this, there are some ideas I want to explore.
I can understand that, I'd like to solve this using composition instead of inheritance. Spectre should allow the user to be free to decide on which reporters they want to use, be it the standard reporter, dot reporter, Xcode reporter or even a JUNIT reporter that outputs to a file. I'd like to alter the architecture to allow having some kind of AggregateReporter which can forward to multiple reports, unfortunately I fear the current reporter design may make this difficult as it is closure based and executing multiple reporters is not simple.
This can be disabled using the following snippet: func removeXCTestLogger() {
let observer = XCTestObservationCenter.shared
if let observers = observer.value(forKeyPath: "observers") as? [XCTestObservation] {
let loggers = observers.filter { NSStringFromClass(type(of: $0)) == "XCTestLog" }
loggers.forEach(observer.removeTestObserver)
}
} However the problem is how to run that code at the right time, as far as I see it. It is not possible in Swift as it's not possible to run code at the right time. It may be possible using mixed sources (Objective-C) though, but that opens up complexity having a separate target to be able to build the source with swift package manager as it doesn't support mixed source in same target (at the moment). |
Would still love for this to be implemented. I'll use your fork and branch for now @ilyapuchka. |
Seeing an issue with this in that when test fail they are reported as failed in Xcode, but the final output shows 0 tests run. That means when running on CI any failures don't actually show up You can see here that the the
|
Resolves #2
This PR adds a very naive support for reporting test failures in Xcode. Instead of using runtime to create tests, as suggested in #17 I've added a method
XCTestCase.describe
that will store a reference to this test case in custom reporter, to use for reporting later, calls global describe function with passed in closure and then runs all the tests. At the end all added tests are removed so that they are not run again later. All individual test failures will appear in Issues navigator.Tests will be still run at the application exit (without reporting via Xcode) but if they were already run with
XCTestCase.describe
they will be removed from the global context and so will not be run twice.Also added option to disable colours in output which make output less readable in Xcode console, that does not support ANSI colors anyway.