Report skipped tests to Xcode

[amomchilov]

The PR should summarize what was changed and why. Here are some questions to
help you if you're not sure:

• What behavior was changed?

  When running on macOS, skipped tests will be reported in the Xcode test navigator, like so:

  

  The skipped line will not be marked in the source code editor. It appears that Xcode only does that for statically defined tests (those marked ), not runtime tests (those marked ), which is what Quick uses exclusively.

  Future direction: when some tests are "focused", should we mark all other tests as skipped?

• What code was refactored / updated to support this change?

  The general error-handling path was extracted into a private reportFailedTest method.

• What issues are related to this PR? Or why was this change introduced?

  Resolves Feature request: Mark skipped tests as skipped in Xcode. #1093

Checklist - While not every PR needs it, new features should consider this list:

• Does this have tests?
• Does this have documentation? Idk, Should it?
• Does this break the public API (Requires major version bump)? No.
• Is this a new feature (Requires minor version bump)?

[amomchilov]

Hey @ikesyo , could you please give some early feedback on my first draft?

Also, do you have any advice on how I should go about testing this?

[ikesyo]

Could you please fix the followings at first?

• indentation: use spaces over tabs
• SwiftLint violations (you can see them in the "Files changed" tab)
  • e.g.

[amomchilov]

Could you please fix the followings at first?

@ikesyo I sure can! (And I did.) Though this is still an early-stages draft. I was less concerned about the linting, I'm worried more about the substance.

I was hoping you could give me some guidance on how to write tests for this.

[jessesquires]

Hey @amomchilov! 👋🏼

I'm a new maintainer here trying to get v5.0 released. This looks like a great change and the diff is pretty small, so I think we can include it.

Are you willing and interested in following up on this fix?

[jessesquires]

I have rebased the branch on main for you

[amomchilov]

Thanks @jessesquires, I'd love to get this moving

Are you willing and interested in following up on this fix?

I am, but I got stuck, and never heard back. My qualm still exists, I don't know to properly cover this with a test.

Could you help with that?

[amomchilov]

I rebased again. There's two outstanding issues:

1. Testing
2. Deciding what to do about QuickSpec.current.testRun. Is it safe to force-unwrap it?

[jessesquires]

Thanks @amomchilov ! 🙌🏼

I rebased again. There's two outstanding issues:

Testing

@younata Do you have any thoughts here?

Deciding what to do about QuickSpec.current.testRun. Is it safe to force-unwrap it?

I left comments in-line.

[jessesquires]

I would probably be ok omitting tests for this since it is a bit odd and doesn't seem like a massive risk.

Also, ignore the Danger check. The failure is expected for forks.

[jessesquires]

Looks like the SwiftPM failures are valid.

[amomchilov]

I would probably be ok omitting tests for this since it is a bit odd and doesn't seem like a massive risk.

There's a certain irony to a test framework not being well tested, isn't there? 😛

Looks like the SwiftPM failures are valid.

Ah yes, NSStringFromSelector is only available to Linux. I gated it.

Let's see what CI says this time

[younata]

Testing
@younata Do you have any thoughts here?

Yes! Similar to how we verify that test failures are reported to Xcode (see ItTests.testImplicitErrorHandling()), we can add another set of fixture tests that do the right thing and then verify with the test run that the correct skip count was recorded.

Something like this (I have not tried to run this code)

// This is the fixture
final class FunctionalTests_SkippingTestsSpec: QuickSpec {
    override func spec() {
        it("supports skipping tests") { XCTSkip("This test is intentionally skipped") }
        it("supports not skipping tests") { }
    }
}

final class ItTests: // ...
    func testSkippingExamplesAreCorrectlyReported() {
        let result = qck_runSpec(FunctionalTests_SkippingTestsSpec.self)!
        XCTAssertTrue(result.hasSucceeded)
        XCTAssertEqual(result.executionCount, 2)
        XCTAssertEqual(result.skipCount, 1)
        XCTAssertEqual(result.totalFailureCount, 0)
    }
    // ...

[jessesquires]

@younata thank you! 🙌🏼

@amomchilov looks like CI is ✅ now! 🎉 Let's just implement @younata's testing suggestion and then we can merge.

[amomchilov]

This fails in some CI configurations, but not others. I can't figure out the cause, any ideas?

[younata]

Maybe the API for marking tests as skipped changed in Xcode 12.5? All those test failures are referencing an Xcode 12.4 toolchain.

I'm ok with skipping these tests (hehe) on Xcode 12.4, especially considering that #1138 means we'll be dropping Xcode <13 anyway.

[jessesquires]

Maybe the API for marking tests as skipped changed in Xcode 12.5? All those test failures are referencing an Xcode 12.4 toolchain.

I vaguely remember seeing some tests that only ran for specific Xcode versions, but maybe that was something else?

Is there a good way to gate the tests to be ignored/skipped on Xcode 12.4?

I'm ok with skipping these tests (hehe) on Xcode 12.4, especially considering that #1138 means we'll be dropping Xcode <13 anyway.

I'm ok with this too, but I'd like to do the following:

1. Let's submit a new PR that drops Xcode 12.4 support. This would mean updating any relevant code, as well as the GH workflows.
2. Merge that PR, then rebase this PR on top of it.

[amomchilov]

I'm ok with skipping these tests (hehe) on Xcode 12.4

How would I do that? Is there a way to make macros around Xcode versions?

EDIT: nvm, i posted this without GitHub refreshing and showing that prev comment

[amomchilov]

I found the issue with Xcode 12.4. Their API was just slightly different:

XCTSkippedTestContext used to have:

• filePath: NSString
• lineNumber: NSString

Which was passed to -[XCTestRun recordSkipWithDescription:inFile:atLine:].

After Xcode 12.4, this filePath and lineNumber was extracted into a new sourceCodeContext: XCTSkippedTestContext property, which is passed directly to a new method: -[XCTestRun recordSkipWithDescription:sourceCodeContext:].

I added a shim to support Xcode 12.4, because it was pretty straight forward. We can ship this now (it should all be green! :D). In the future, if we want to go out of our way to drop 12.4, we can delete this shim

[jessesquires]

Thanks so much @amomchilov ! 🙌🏼

[amomchilov]

Ah, I forgot to make some Objective-C dependant code to be guarded by canImport(Darwin). Fixed.

[amomchilov]

Third time's the charm?

[amomchilov]

Woohoo, we're green!

@ikesyo requested changes a while ago, but apart from that, I think this is ready to merge

[amomchilov]

@jessesquires What do we do if @ikesyo doesn't respond to re-review this?

[jessesquires]

What do we do if @ikesyo doesn't respond to re-review this?

It's all good, no worries! We only need 1 approval.

I'll merge this tomorrow -- and potentially tag the v5.1 release (still deciding on that and coordinating with Nimble).

[ikesyo]

LGTM! 👏

[amomchilov]

@jessesquires Was it decided whether this will be tagged 5.1, or when it would be merged?

[jessesquires]

Thanks @amomchilov ! 💯

Decided to just keep this for v6.0 -- unfortunately I don't have capacity to do a v5.1 right now, and we haven't landed any other big changes. 😊

[jessesquires]

But, there isn't a ton of work to do for v6.0 -- so we can probably get that out within the next few months.

[jessesquires]

@amomchilov invited you to be a contributor 😄