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

SR-1417: Add non-optional overloads of XCTAssertEqual and XCTAssertNo… #2551

Closed
wants to merge 1 commit into from
Closed

Conversation

nsalmoria
Copy link
Contributor

@nsalmoria nsalmoria commented May 16, 2016

What's in this pull request?

Resolved bug number: (SR-1417)


Before merging this pull request to apple/swift repository:

  • Test pull request on Swift continuous integration.

Triggering Swift CI

The swift-ci is triggered by writing a comment on this PR addressed to the GitHub user @swift-ci. Different tests will run depending on the specific comment that you use. The currently available comments are:

Smoke Testing

Platform Comment
All supported platforms @swift-ci Please smoke test
OS X platform @swift-ci Please smoke test OS X platform
Linux platform @swift-ci Please smoke test Linux platform

Validation Testing

Platform Comment
All supported platforms @swift-ci Please test
OS X platform @swift-ci Please test OS X platform
Linux platform @swift-ci Please test Linux platform

Note: Only members of the Apple organization can trigger swift-ci.

…tEqual

Previously, the only version of the functions that accepted values was the one that implicitly wraps them into Optionals. This generated a confusing error message when the assert failed. Having a separate overload that accepts non-optional types ensures that the correct description is printed when the assert fails.

Example:
XCTAssertEqual(1, 2, "message")

Previous error:
XCTAssertEqual failed: ("Optional(1)") is not equal to ("Optional(2)") - message

New error:
XCTAssertEqual failed: ("1") is not equal to ("2") - message

…tEqual

Previously, the only version of the functions that accepted values was the one that implicitly wraps them into Optionals. This generated a confusing error message when the assert failed. Having a separate overload that accepts non-optional types ensures that the correct description is printed when the assert fails.
@nsalmoria
Copy link
Contributor Author

@modocache I think I did what you asked in swiftlang/swift-corelibs-xctest#110

expectEqual(0, failingTestRun.unexpectedExceptionCount)
expectEqual(1, failingTestRun.totalFailureCount)
expectFalse(failingTestRun.hasSucceeded)
}
Copy link
Contributor

@modocache modocache May 16, 2016

Choose a reason for hiding this comment

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

Is there a way to test that your overloads accomplish the stated goal? In other words, could we check the failure message?

I think this is possible by using XCTestObservationCenter:

// pseudocode, not sure if this will work
class MyObserver: XCTestObservation {
  func testCase(_ testCase: XCTestCase, didFailWithDescription description: String, inFile filePath: String?, atLine lineNumber: UInt) {
    expectEqual(description, "XCTAssertEqual failed: (\"1\") is not equal to (\"2\") - ")
  }
}
let observer = MyObserver()
XCTestObservationCenter.center().addTestObserver(observer)
let failingTestCase = AssertEqualOptionalTestCase(selector: #selector(AssertEqualOptionalTestCase.test_whenOptionalsAreNotEqual_fails))
execute(failingTestCase.run)
XCTestObservationCenter.center().removeTestObserver(observer)

@modocache
Copy link
Contributor

This is excellent, thanks!! I'd like to see tests that prevent this from regressing in the future. I left a comment to demonstrate one potential way of writing such a test.

@modocache
Copy link
Contributor

@swift-ci please test

@nsalmoria
Copy link
Contributor Author

I'd like to see tests too but I'm afraid that's beyond me for the time being :-/

@modocache
Copy link
Contributor

Have you tried my comment above? #2551 (comment) I think that might work...

@nsalmoria
Copy link
Contributor Author

The problem is that my Mac seems to be significantly underpowered for Swift development, both in terms of CPU power and of mass storage space (it's a 2012 MacBook Air with 120GB drive).

In addition, the XCTest tests seem to be flat out disabled at the moment from the standard suite.

Is there a way to quickly build and run just a single test?

@modocache
Copy link
Contributor

In addition, the XCTest tests seem to be flat out disabled at the moment from the standard suite.

Ah yes, this is true. Removing this line will re-enable the tests.

There is, unfortunately, no great way to run only one test at the moment; you'll need to run something like swift/utils/build-script --R --validation-test once. After the initial run, if you've only modified the test file, you can run llvm/utils/lit/lit.py build/Ninja-ReleaseAssert/swift-macosx-x86_64/validation-test-macosx-x86_64/stdlib/XCTest.swift to run just the modified test.

Yeah, it does take a bit of a workhorse computer to contribute to Swift... I can help with the tests if you don't mind; I'll take your commit and put some tests in a separate commit on top of it, then submit a pull request.

@nsalmoria
Copy link
Contributor Author

I would gladly appreciate your help :-)

@jckarter
Copy link
Contributor

Within the build directory, you can run llvm-lit -sv test-<platform>/path/to/test.swift to run only test/path/to/test.swift manually.

@briancroom
Copy link
Contributor

I'm not seeing any activity here but would love to see this go in. @modocache @nsalmoria any objections if I go ahead and add the tests we are looking for here?

@modocache
Copy link
Contributor

@briancroom Yes, go for it! Sorry, this was at the back of my mind all week but never found the time. Thanks for pinging!

@nsalmoria
Copy link
Contributor Author

@briancroom please go ahead! Thank you!

@briancroom
Copy link
Contributor

Just opened #2788

@modocache
Copy link
Contributor

@nsalmoria I think this pull request can be closed in favor of @briancroom's #2788. That pull request includes your commit 👍

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.

4 participants