XCTestAssertNoThrow #184
XCTestAssertNoThrow #184
Conversation
|
cc @briancroom @modocache for visibility. |
|
@swift-ci please test |
|
Looks like the Linux CI job has some build errors: Maybe the compiler crashes instead of emitting errors when run on macOS…? We’ll have to wait and see what the macOS CI job says. If it does crash instead of simply exiting due to compilation errors, that might be a good bug to file. |
|
@modocache Fixed 2 of the 2 issues related to XCTAssert. The third issue you highlighted is in WallClockTimeMetric.swift and beyond the scope of the PR. Also, this test Still getting compilation errors on my mac, so if you want, I can open up a bug? Should I do it on bugs.swift.org? |
|
Ah yes, it's my understanding that master is currently broken. We're waiting on getting apple/swift-corelibs-foundation#785 merged. |
|
Alright, so there's nothing more I can do at this point in time, correct? |
There was a problem hiding this comment.
No, there's still some work that needs to be done here. apple/swift-corelibs-foundation#785 has been merged, but there are some errors in your tests.
| } | ||
|
|
||
| // CHECK: Test Case 'ErrorHandling.test_shouldThrowErrorDefiningError' started at \d+:\d+:\d+\.\d+ | ||
| // CHECK: Test Case 'ErrorHandling.test_shouldThrowErrorDefiningError' passed \(\d+\.\d+ seconds\) |
There was a problem hiding this comment.
Oh, I'm sorry, I scanned over this and didn't realize this was a failing test. If this test is meant to fail, then these assertions are incorrect, and will fail the swift-corelibs-xctest test suite.
Here, these CHECK statements are assertions that the text "Test Case 'ErrorHandling.test_shouldThrowErrorDefiningError' passed" must appear in the test output. Since test_shouldThrowErrorDefiningError will fail, this output will definitely not appear in the output.
I'd like for you to add a CHECK assertion to verify that XCTAssertNoThrow(try functionThatDoesThrowError()) will fail a test case. Other tests in this file, such as test_throwsErrorInAssertionButFailsWhenCheckingError, are examples of failure assertions.
There was a problem hiding this comment.
ok, I think I understand. I'll look closely at what the other failing tests are doing. I may have copy-pasted info from the wrong example.
| _ = try expression() | ||
| return .success | ||
| } catch _ { | ||
| return .expectedFailure("threw error") |
There was a problem hiding this comment.
I think we're going to want to include a textual description of the error that was thrown here. For comparison, the overlay implementation looks like this.
Making this change will impact how you specify the expected failure message in the functional test suite, as @modocache was discussing in his comment.
There was a problem hiding this comment.
@briancroom Re; textual description, I was just following what was done for XCTAsserThrowsError:
I'm more than happy to make it more descriptive as I did in the overlay you referenced.
There was a problem hiding this comment.
Ah yes, I see.
I think the failure is terser for XCTAssertThrowsError because the error being reported is the lack of an error, when an error was expected, so there really isn't more to report. In the case of this new function, we can and should provide whatever details we can about the unexpected error that was encountered.
There was a problem hiding this comment.
Agreed! Will do!
…follows Swift's XCTest format
|
Those expressions are evaluated as the current line number in the file, plus a constant integer, allowing you to refer to a relative line in the file. This way the assertion remains valid when lines are inserted or deleted earlier in the file. Although we aren't using the real FileCheck program here for evaluating the output from the functional tests, the approach and this syntax are effectively cribbed from that tool. |
| _ = try expression() | ||
| return .success | ||
| } catch _ { | ||
| return .expectedFailure("\(filename):\(line): XCTAssertNoThrow failed: threw error \"\(error)\". : \(message)") |
There was a problem hiding this comment.
Note that a lot of the pieces of this failure message are produced in the _XCTEvaluateAssertion function and won't need to be included on this line here. I think you'll want to end up with with an expression like:
return .expectedFailure("threw error \"\(error)\"")
| } | ||
|
|
||
| // CHECK: Test Case 'ErrorHandling.test_shouldThrowErrorDefiningError' started at \d+:\d+:\d+\.\d+ | ||
| // CHECK: .*/ErrorHandling/main.swift:[[@LINE+3]]: error: ErrorHandling.test_shouldThrowErrorDefiningFailure : XCTAssertNoThrow failed: threw error - |
There was a problem hiding this comment.
One thing to keep in mind while you're working on the contents of this CHECK line is that the string is interpreted as a regex, so you may need to escape some characters to get the behavior you're looking for.
|
I left a couple of pointers for while you're working on this @ArtSabintsev! Also note that you should be able to develop from inside of the Xcode IDE, if that's a comfortable environment for you. You'd want to run the |
|
@briancroom Love Xcode :) Good to know that that's the best way to test it. Been a bit of a pain using this like a text editor without the compiler and rest of the SDKs for code completion/automation. |
|
Just to confirm @briancroom, I can use the toolchain that ships with Xcode 8.2, correct? According to https://swift.org/download/#latest-development-snapshots, it says I can. However, when building any of the targets in the XCTest.workspace, I am seeing the attached errors. |
|
@ArtSabintsev The |
|
@ArtSabintsev The |
|
@ikesyo alright, I'll merge master into my branch and use a newer snapshot of swift. Thanks! |
|
@swift-ci please test |
|
I'm seeing this again: Should I be adding an extra And only in the failure case? Or in both cases? |
|
Ah, yes, the failure message will have a trailing hyphen. There's a case to be made that it shouldn't be there in the case that there's no content after it, but that's how the failure messages are currently produced both here and in the overlay. After the hyphen would come the user-supplied |
|
@briancroom Added the missing hyphen. 🤞 that it passes this time :) |
|
@swift-ci please test |
1 similar comment
|
@swift-ci please test |
|
@briancroom @modocache looks like CI failed, but I can't deduce why. It failed fast as well. |
|
Yeah the bots seem to be running into some odd trouble. @shahmishal do you know what's going on here? |
|
@swift-ci test OS X |
|
still seems to be broken @shahmishal @briancroom |
|
Looking into the issue. |
|
@ArtSabintsev / @briancroom Fixed. Sorry for the trouble. |
|
@swift-ci please test |
|
Thanks, @shahmishal! |
1 similar comment
|
Thanks, @shahmishal! |
There was a problem hiding this comment.
Looks like the expectation on the total number of tests isn't right. The actual output is "8 tests, with 5 failures", but the CHECK statement expects "6 tests, with 4 failures". You'll need to update the check statement at the bottom of the test.
Actual: '\t Executed 8 tests, with 5 failures (2 unexpected) in 0.02 (0.02) seconds\n'
Expected: '\\t Executed 6 tests, with 4 failures \\(2 unexpected\\) in \\d+\\.\\d+ \\(\\d+\\.\\d+\\) seconds'
|
@modocache I made that change yesterday: https://github.com/apple/swift-corelibs-xctest/pull/184/files#diff-bb5b3151a818571b84c05803e281747fR134 per @briancroom's suggestion. |
|
Oh, I see. It's @briancroom's other comment above, that's causing the failure: https://github.com/apple/swift-corelibs-xctest/pull/184/files#r97010710. You'll need to update that spot, too. |
|
@briancroom @modocache Thanks! I must have missed that comment in this chain. Done - Good to go now! |
|
@swift-ci please test |
|
@briancroom I don't think CI started. |
|
@swift-ci please test |
|
@briancroom looks like everything passed! |
There was a problem hiding this comment.
I think this is ready to go! Thanks for sticking with the review @ArtSabintsev.
|
I think it makes sense to get this into Swift 3.1 as well so that it will ship with the function available on all platforms, since it already made it into the overlay. @ArtSabintsev could you open a new PR with this change that targets |
|
sure will happily do that! |
|
Done! #185 |
I get the following errors when attempting to test my branch. I might be doing something wrong.