Skip to content
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

Closed
wants to merge 4 commits into from
Closed

Xcode reports #30

wants to merge 4 commits into from

Conversation

ilyapuchka
Copy link

@ilyapuchka ilyapuchka commented Dec 25, 2017

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.

screen shot 2017-12-25 at 17 45 57

@yonaskolb
Copy link

Awesome! 👏

Copy link
Owner

@kylef kylef left a 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.

var description:String {
if isatty(STDOUT_FILENO) > 0 {
if isatty(STDOUT_FILENO) > 0 && ANSI.isEnabled {
Copy link
Owner

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

Copy link
Owner

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.

reporter = StandardReporter()
#else
reporter = XCTestCase.reporter
Copy link
Owner

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")
}
}
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: Trailing whitespace

}

}

Copy link
Owner

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.

Copy link
Author

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.

@@ -34,6 +34,15 @@ class Context : ContextType, CaseType {
var afters = [After]()

init(name: String, disabled: Bool = false, parent: Context? = nil) {
var name = name
Copy link
Owner

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.

@ilyapuchka
Copy link
Author

ilyapuchka commented Dec 27, 2017

@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 swift test there will be only XCTest output.
Do you want to continue with #32 or do you want me to address your feedback here?

# Conflicts:
#	Sources/Spectre/Reporters.swift
@ilyapuchka ilyapuchka force-pushed the xcode-reports branch 2 times, most recently from 7021d55 to 331232e Compare December 30, 2017 14:00
@yonaskolb
Copy link

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

  • show test failures in XcodeUI (fantastic)
  • keeping custom output (nice to have)
  • being able to run individual tests just by clicking on them in the UI (is that even possible)

@kylef
Copy link
Owner

kylef commented Feb 6, 2018

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.

Looks nice, though my idea was to keep standard reporter output, I find Xcode output much less readable.

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.

But I didn't find a way to suppress it, so it still interferes with standard reporter output a bit.

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).

@ilyapuchka ilyapuchka closed this Jun 6, 2018
@yonaskolb
Copy link

Would still love for this to be implemented. I'll use your fork and branch for now @ilyapuchka.

@yonaskolb
Copy link

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 All tests suite fails, but all in all there are 0 runs

Test Suite 'All tests' failed at 2018-07-18 04:05:16.296.
	 Executed 13 tests, with 1 failure (1 unexpected) in 2.449 (2.453) seconds
0 passes and 0 failures

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants