Skip to content

Various minor changes to XCTest-files. #80

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.

Sign up for GitHub

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

Merged
merged 8 commits into from
Mar 26, 2016
Merged

Various minor changes to XCTest-files. #80

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
0d912f3
Added backticks in a comment.
marcusrossel Mar 23, 2016
bdeb15b
Added backticks in comments.
marcusrossel Mar 23, 2016
6ff1cb0
Updated `selectedTestFilter` property.
marcusrossel Mar 23, 2016
cfdf9fb
Updated `func filterTests`.
marcusrossel Mar 23, 2016
30c1133
Updated multiple switch-statements.
marcusrossel Mar 23, 2016
da99bbb
Updated XCTestCase.swift in various ways.
marcusrossel Mar 23, 2016
17a2b03
Added a missing whitespace.
marcusrossel Mar 23, 2016
e0f34a7
Adjusted code indenting.
marcusrossel Mar 23, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 10 additions & 15 deletions Sources/XCTest/TestFiltering.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
//
//
// XCTestFiltering.swift
// This provides utilities for executing only a subset of the tests provided to XCTMain
// This provides utilities for executing only a subset of the tests provided to `XCTMain`
//

internal typealias TestFilter = (XCTestCase.Type, String) -> Bool
Expand All @@ -21,15 +21,10 @@ internal struct TestFiltering {
}

var selectedTestFilter: TestFilter {
if let selectedTestName = selectedTestName {
if let selectedTest = SelectedTest(selectedTestName: selectedTestName) {
return selectedTest.matches
} else {
return excludeAllFilter()
}
} else {
return includeAllFilter()
}
guard let selectedTestName = selectedTestName else { return includeAllFilter() }
guard let selectedTest = SelectedTest(selectedTestName: selectedTestName) else { return excludeAllFilter() }

return selectedTest.matches
Copy link
Contributor

Choose a reason for hiding this comment

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

@briancroom Any opinions here? I find both equally readable.

Copy link
Contributor

Choose a reason for hiding this comment

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

I like this change! The flatter structure is nice.

Copy link

Choose a reason for hiding this comment

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

Me too. Shorter and less nested.

}

private func excludeAllFilter() -> TestFilter {
Expand All @@ -42,12 +37,12 @@ internal struct TestFiltering {

static func filterTests(entries: [XCTestCaseEntry], filter: TestFilter) -> [XCTestCaseEntry] {
return entries
.map({ testCase, tests in
return (testCase, tests.filter({ filter(testCase, $0.0) }))
})
.filter({ testCase, tests in
.map { testCase, tests in
return (testCase, tests.filter { filter(testCase, $0.0) } )
}
.filter { testCase, tests in
return !tests.isEmpty
})
}
Copy link
Contributor

Choose a reason for hiding this comment

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

@briancroom Same here. I don't have a strong opinion, I think either way of writing it is fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm pretty ambivalent about this one. I've been pretty happy following the Rule of Kevin in my Swift code, but I don't have a strong opinion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, I've searched some files in swift/stdlib/public/core to try to find occurrences of the string "})" (which would indicate the use of non-trailing syntax). I found some cases:

return split(
  maxSplits: maxSplits,
  omittingEmptySubsequences: omittingEmptySubsequences,
  isSeparator: { $0 == separator })

else if let offset = (e as? Int).map({ IntMax($0) }) ?? (e as? IntMax) { //... }

...

I'm not quite sure in terms of consistency, when it comes to use of trailing closures.
But any use of non-trailing syntax would indicate to me, that there are appropriate situations for its use. I'm therefore leaning towards the Rule Of Kevin.

}
}

Expand Down
29 changes: 11 additions & 18 deletions Sources/XCTest/XCTAssert.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,25 +52,19 @@ private enum _XCTAssertionResult {
case UnexpectedFailure(ErrorProtocol)

var expected: Bool {
switch (self) {
case .UnexpectedFailure(_):
return false
default:
return true
switch self {
case .UnexpectedFailure(_): return false
Copy link
Contributor

Choose a reason for hiding this comment

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

In #79, you made changes that placed a line break after a case statement. Here you've removed line breaks. Do you have a clear goal in mind here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to explain my intent in the commit.
When the expression(s) inside of a case fit on one line (for all the cases in the switch), I moved them next to the case-label.
If any case required multiple lines, I added a line break.
This results in consistency within a switch-statement, but not necessarily across different switches.

I use this style in my own projects, as it's consistent, but also reduces the size of switches, with short expressions in each case.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense to me. Thanks, and sorry I didn't realize you had already explained in the commit messages!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool.
Just as a side-question: Is there way to make a commit that affects multiple files? I'm editing the code online, so when I try to switch to another file, it would not save the changes made to the current file.

default: return true
}
}

func failureDescription(assertion: _XCTAssertion) -> String {
let explanation: String
switch (self) {
case .Success:
explanation = "passed"
case .ExpectedFailure(let details?):
explanation = "failed: \(details)"
case .ExpectedFailure(_):
explanation = "failed"
case .UnexpectedFailure(let error):
explanation = "threw error \"\(error)\""
switch self {
case .Success: explanation = "passed"
case .ExpectedFailure(let details?): explanation = "failed: \(details)"
case .ExpectedFailure(_): explanation = "failed"
case .UnexpectedFailure(let error): explanation = "threw error \"\(error)\""
}

if let name = assertion.name {
Expand All @@ -92,23 +86,22 @@ private func _XCTEvaluateAssertion(assertion: _XCTAssertion, @autoclosure messag
switch result {
case .Success:
return

default:
if let handler = XCTFailureHandler {
handler(XCTFailure(message: message(), failureDescription: result.failureDescription(assertion), expected: result.expected, file: file, line: line))
}
}
}

/// This function emits a test failure if the general Bool expression passed
/// to it evaluates to false.
/// This function emits a test failure if the general `Boolean` expression passed
/// to it evaluates to `false`.
Copy link

Choose a reason for hiding this comment

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

I would suggest either keeping it "Bool" and adding the backticks since that is a type name or changing to Boolean and not adding the backpacks since then it's just prose...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand your point. What I don't understand though is: if Boolean is not a defined type, how can it be used as one?:
@autoclosure expression: () throws -> Boolean

This code was taken from XCTAssert.swift

Copy link

Choose a reason for hiding this comment

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

You're absolutely right! I forgot about the Boolean protocol as distinct from the Bool concrete type.

Never mind!

Copy link
Contributor

Choose a reason for hiding this comment

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

Possibly contributing to the confusion is the fact that this was previously named BooleanType before the recent Swift 3 stdlib naming changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I see. I haven't been aware of that name change.
So, I'll keep the changes, as they were (if that's the consensus).

///
/// - Requires: This and all other XCTAssert* functions must be called from
/// within a test method, as passed to `XCTMain`.
/// Assertion failures that occur outside of a test method will *not* be
/// reported as failures.
///
/// - Parameter expression: A boolean test. If it evaluates to false, the
/// - Parameter expression: A boolean test. If it evaluates to `false`, the
/// assertion fails and emits a test failure.
/// - Parameter message: An optional message to use in the failure if the
/// assertion fails. If no message is supplied a default message is used.
Expand Down
21 changes: 9 additions & 12 deletions Sources/XCTest/XCTestCase.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,13 +47,13 @@ public class XCTestCase {
/// the signature required by `XCTMain`
/// - seealso: `XCTMain`
public func testCase<T: XCTestCase>(allTests: [(String, T -> () throws -> Void)]) -> XCTestCaseEntry {
let tests: [(String, XCTestCase throws -> Void)] = allTests.map({ ($0.0, test($0.1)) })
let tests: [(String, XCTestCase throws -> Void)] = allTests.map { ($0.0, test($0.1)) }
return (T.self, tests)
}

private func test<T: XCTestCase>(testFunc: T -> () throws -> Void) -> XCTestCase throws -> Void {
return { testCaseType in
guard let testCase: T = testCaseType as? T else {
guard let testCase = testCaseType as? T else {
fatalError("Attempt to invoke test on class \(T.self) with incompatible instance type \(testCaseType.dynamicType)")
}

Expand All @@ -62,7 +62,7 @@ private func test<T: XCTestCase>(testFunc: T -> () throws -> Void) -> XCTestCase
}

// FIXME: Expectations should be stored in an instance variable defined on
// XCTestCase, but when so defined Linux tests fail with "hidden symbol
// `XCTestCase`, but when so defined Linux tests fail with "hidden symbol
// isn't defined". Use a global for the time being, as this seems to
// appease the Linux compiler.
private var XCTAllExpectations = [XCTestExpectation]()
Expand All @@ -74,7 +74,10 @@ extension XCTestCase {
return true
}
set {
// TODO: When using the Objective-C runtime, XCTest is able to throw an exception from an assert and then catch it at the frame above the test method. This enables the framework to effectively stop all execution in the current test. There is no such facility in Swift. Until we figure out how to get a compatible behavior, we have decided to hard-code the value of 'true' for continue after failure.
// TODO: When using the Objective-C runtime, XCTest is able to throw an exception from an assert and then catch it at the frame above the test method.
// This enables the framework to effectively stop all execution in the current test.
// There is no such facility in Swift. Until we figure out how to get a compatible behavior,
// we have decided to hard-code the value of 'true' for continue after failure.
}
}

Expand Down Expand Up @@ -143,14 +146,8 @@ extension XCTestCase {
}
}

var testCountSuffix = "s"
if tests.count == 1 {
testCountSuffix = ""
}
var failureSuffix = "s"
if totalFailures == 1 {
failureSuffix = ""
}
let testCountSuffix = (tests.count == 1) ? "" : "s"
let failureSuffix = (totalFailures == 1) ? "" : "s"

XCTPrint("Executed \(tests.count) test\(testCountSuffix), with \(totalFailures) failure\(failureSuffix) (\(unexpectedFailures) unexpected) in \(printableStringForTimeInterval(totalDuration)) (\(printableStringForTimeInterval(overallDuration))) seconds")
}
Expand Down
8 changes: 4 additions & 4 deletions Sources/XCTest/XCTestMain.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,11 @@
//

#if os(Linux) || os(FreeBSD)
import Glibc
import Foundation
import Glibc
import Foundation
#else
import Darwin
import SwiftFoundation
import Darwin
import SwiftFoundation
#endif

internal func XCTPrint(message: String) {
Expand Down