Implementation for XCTAssertNoThrow #6776

ArtSabintsev · 2017-01-13T05:45:07Z

Implements XCTAssertNoThrow, as it's available in Objective-C, but missing in Swift.

I used XCTAssertThrowsError as a guideline, implementing the bulk of this method and validation test by doing the inverse of what I saw there.

First time adding functionality to Swift, so not 100% sure on how to test any of this locally.

Is there a way to test the validation locally on my machine?

cc @slavapestov @modocache

…TAssertNoThrow

CodaFi · 2017-01-13T05:52:07Z

You can run this particular validation test locally from the parent directory of your swift checkout:

./llvm/utils/lit/lit.py -sv ./build/Ninja-DebugAssert/swift-macosx-x86_64/validation-test-macosx-x86_64/stdlib/XCTest.swift

CodaFi · 2017-01-13T05:57:16Z

stdlib/public/SDK/XCTest/XCTest.swift

@@ -1030,6 +1030,40 @@ public func XCTAssertThrowsError<T>(_ expression: @autoclosure () throws -> T, _
  }
 }

+public func XCTAssertNoThrow<T>(_ expression: @autoclosure () throws -> T, _ message: @autoclosure () -> String = "", file: StaticString = #file, line: UInt = #line, _ errorHandler: (_ error: Error) -> Void = { _ in }) {


The indentation of this function should match the rest of this file.

Fixed this, but this request won't close, as it was done on a line of code that itself didn't need a change.

I guess it's a good thing because you only fixed one level of indentation! If you're using Xcode, set your indentation settings appropriately and try highlighting the regions I mentioned. Hit ^+I and it'll reindent for you.

CodaFi · 2017-01-13T05:59:45Z

stdlib/public/SDK/XCTest/XCTest.swift

+            return
+        }
+
+        _XCTRegisterFailure(true, "XCTAssertNoThrow failed: threw error \"\(caughtError)\"", message, file, line)


Shouldn't the invariant here be that if .success is returned from the _XCTRunThrowableBlock call then caughtErrorOptional should be .none? It looks like _XCTRunThrowableBlock already handles catching Swift exceptions so you should be able to delete most of the preceding code and get down to essentially

let result = _XCTRunThrowableBlock { _ = try expression() }

Simplified, it looks like this, right?

public func XCTAssertNoThrow<T>(_ expression: @autoclosure () throws -> T, _ message: @autoclosure () -> String = "", file: StaticString = #file, line: UInt = #line) { let assertionType = _XCTAssertionType.assertion_NoThrow let result = _XCTRunThrowableBlock { _ = try expression() } switch result { case .success: guard let error = error else { return } case .failedWithError(let error): _XCTRegisterFailure(true, "XCTAssertNoThrow failed: threw error \"\(error)\"", message, file, line) case .failedWithException(_, _, let reason): _XCTRegisterFailure(true, _XCTFailureDescription(assertionType, 1, reason as NSString), message, file, line) case .failedWithUnknownException: _XCTRegisterFailure(true, _XCTFailureDescription(assertionType, 2), message, file, line) } }

I got rid of some extraneous stuff as well.

Pushed that change to make it easier for you to see.

CodaFi · 2017-01-13T06:03:49Z

validation-test/stdlib/XCTest.swift

@@ -252,6 +252,75 @@ XCTestTestSuite.test("XCTAssertThrowsError") {

 }

+XCTestTestSuite.test("XCTAssertNoThrow") {
+    class ErrorTestCase: XCTestCase {


Another indentation fix. We try to use 2-space indents here. The other Swift-related projects may have different styles.

CodaFi · 2017-01-13T06:04:58Z

Modulo the nits above, this LGTM.

ArtSabintsev · 2017-01-13T06:08:57Z

Thanks - just got llvm installed. Going to try to run this locally and then address your concerns.

ArtSabintsev · 2017-01-13T06:26:12Z

@CodaFi I pushed the change I left in the comment:

public func XCTAssertNoThrow<T>(_ expression: @autoclosure () throws -> T, _ message: @autoclosure () -> String = "", file: StaticString = #file, line: UInt = #line) {
  let assertionType = _XCTAssertionType.assertion_NoThrow

  let result = _XCTRunThrowableBlock { _ = try expression() }

  switch result {
  case .success:
      guard let error = error else {
          return
      }

  case .failedWithError(let error):
      _XCTRegisterFailure(true, "XCTAssertNoThrow failed: threw error \"\(error)\"", message, file, line)

  case .failedWithException(_, _, let reason):
      _XCTRegisterFailure(true, _XCTFailureDescription(assertionType, 1, reason as NSString), message, file, line)

  case .failedWithUnknownException:
      _XCTRegisterFailure(true, _XCTFailureDescription(assertionType, 2), message, file, line)
  }
}

I think that's what you were referring to.

Still having issues with running tests locally with llvm, but that's neither here nor there. Mind if we smoke test and see what CI says? If it fails, I'll try again in the morning.

CodaFi · 2017-01-13T06:26:35Z

stdlib/public/SDK/XCTest/XCTest.swift

+  let result = _XCTRunThrowableBlock { _ = try expression() }
+
+  switch result {
+  case .success:


It's not enough to just outdent. If you use Xcode you can set your indentation settings appropriately, highlight these code blocks, and hit ^+I (CNTRL+I) to have it do this for you.

yup, was using xcode. Will try that now.

The tests need to be updated as well

CodaFi · 2017-01-13T06:28:28Z

stdlib/public/SDK/XCTest/XCTest.swift

+  case .success:
+      guard let error = error else {
+          return
+      }


This code is dead (and won't compile). Can you remove it and make this case simply return?

Ah, whoops!

CodaFi · 2017-01-13T06:30:29Z

stdlib/public/SDK/XCTest/XCTest.swift

+
+  switch result {
+  case .success:
+    guard let error = error else {


Re-iterating because indent changes collapsed my comment:

This code is dead (and won't compile). Can you remove it and make this case simply return?

CodaFi · 2017-01-13T06:31:37Z

Thanks for sticking with me through the review. I'll trigger CI once the changes settle down. Thank you for putting in the work for this!

ArtSabintsev · 2017-01-13T06:31:52Z

Pushed the change to the success condition as well.

CodaFi · 2017-01-13T06:33:03Z

There are still alignment issues in the tests you added. For example.

ArtSabintsev · 2017-01-13T06:33:48Z

you've got to be kidding me. I did the CTROL+I trick after setting everything to 2 spaces.

I'm a 4 space guy :)

Looking again.

ArtSabintsev · 2017-01-13T06:34:39Z

Ah, that's not my code :)

Want me to fix the alignment it in this PR?

ArtSabintsev · 2017-01-13T06:35:19Z

Oh weird, it's saying it is my code, eventhough I don't recall touching it. Fixing now.

CodaFi · 2017-01-13T06:35:39Z

You mentioned you were having trouble running the tests locally: Do you have a build of the swift compiler and standard library already?

…e 2 spaces instead of 4

CodaFi · 2017-01-13T06:37:40Z

Whoa! Can you please revert that last change and only unindent what you accidentally indented? We have a policy against huge whitespace changes like this.

ArtSabintsev · 2017-01-13T06:37:51Z

sure no problem.

…ests have 2 spaces instead of 4" This reverts commit 43a7b31.

CodaFi · 2017-01-13T07:36:33Z

I'm talking about this

dynamic func test_throws()

The entire implementation is just

dynamic func test_throws() {
  XCTAssertNoThrow(try throwSomething())
}

ArtSabintsev · 2017-01-13T07:38:04Z

Ah, lemme make the change then.

As I said at the onset, was working blindly and on the inverse of what XCTAssertThrowsError did :\

ArtSabintsev · 2017-01-13T07:39:35Z

Oh, right! I got rid of the block that returned errors in the definition, but forgot to do it in the test.

ArtSabintsev · 2017-01-13T07:40:34Z

ok, @CodaFi, pushed.

CodaFi · 2017-01-13T07:41:17Z

The last thing to do is change the constant to

_XCTAssertionType.noThrow

ArtSabintsev · 2017-01-13T07:43:16Z

Done

CodaFi · 2017-01-13T07:43:47Z

The linux tests are going to pass regardless, so

@swift-ci please smoke test Linux platform.

ArtSabintsev · 2017-01-13T07:44:08Z

why's that?

CodaFi · 2017-01-13T07:45:30Z

Because this test is enabled explicitly only for macOS!

@swift-ci please test OS X platform.

ArtSabintsev · 2017-01-13T07:48:48Z

Ah, good to know.

CodaFi · 2017-01-13T08:39:23Z

Welcome to Swift, @ArtSabintsev

⛵️

ArtSabintsev · 2017-01-13T13:19:30Z

Thank you so much for your assistance and for the welcome! Really appreciate it, @CodaFi.

Also, thank you @slavapestov and @modocache!

jrose-apple · 2017-01-13T17:10:17Z

Ah, @CodaFi, in the future please include an Apple XCTest person for overlay modifications. XCTest, Dispatch, and Foundation are somewhat special because of the corelibs projects, but my understanding is that their interfaces are still considered part of the Apple SDK, not part of the Swift Open Source project. I wouldn't expect any problems here given that this is merely an Objective-C API equivalent, but it's still an important part of the process. cc @mike-ferris-apple

@ArtSabintsev, I'm sorry to catch you up in this. :-)

ArtSabintsev · 2017-01-13T17:14:58Z

no problem - this is all very interesting to me!

@jrose-apple If I wanted to implement some or all of these (https://github.com/ArtSabintsev/swift/blob/5008ed84218ba560198a74af6929dfc40f273883/stdlib/public/SDK/XCTest/XCTest.swift#L1053-L1085), what would be the appropriate way to open future PRs? Should I tag specific people and/or approach those in a different manner, since I, too, think these have Objective-C API equivalents.

CodaFi · 2017-01-13T17:25:40Z

@jrose-apple My bad

jrose-apple · 2017-01-13T17:49:30Z

Generally it's always acceptable to mention the code owner for a particular component, who can then direct you to the right person. However, that file doesn't really cover the overlays (stdlib/public/SDK/) since those are mostly owned by people at Apple who don't participate in the Swift Open Source process at all. In this case you'd probably have to wait for one of us to tag in someone we know works on XCTest.

(There's always the technique of looking at who's last modified those files, and seeing who merged those pull requests too. But you shouldn't have to go looking; it should be easy.)

I'll get the file updated for Dispatch, Foundation, and XCTest, and bring this up on the swift-dev list; it should be clearer what to do.

@mike-ferris-apple, for Arthur's specific plan, who should be reviewing these? Since they're all existing functions that are marked unimplemented, do you want to consider them up for general review?

modocache · 2017-01-13T17:50:47Z

/cc @briancroom as well

ArtSabintsev · 2017-01-13T17:59:56Z

Sounds good, @jrose-apple.

By the way, I am in no way consigning myself to implementing all of those functions. Just wanted to know what the process was IF I went through with it ;)

But thanks for all the info!

ArtSabintsev · 2017-01-13T18:18:59Z

@jrose-apple If you'd like, I can implement the same function in XCTest and Foundation, plus I think XCTestAssertThrows is missing in one or both of those.

Thoughts? If so, I can do it tonight or this weekend as I have the entire environment setup (finally), thanks to @slavapestov's guidance.

briancroom · 2017-01-13T20:16:05Z

Hi @ArtSabintsev, thanks for your contribution! This looks great. Thanks also @CodaFi and others involved in the review.

Please do tag me in future PRs for XCTest changes, either here in the XCTest framework overlay, or in Corelibs!

Regarding the other assertion functions:

I don't think it will be possible to implement most (any?) of these in Corelibs because they are all intended to test for ObjC exceptions being thrown or not thrown, which don't exist at all on other platforms. Some of the naming around these is a bit unfortunate because the term "throws" means something a bit different in each language.
I'd be happy to review PRs for implementations of these in the framework overlay, though!

modocache · 2017-01-13T20:58:58Z

Some of the naming around these is a bit unfortunate because the term "throws" means something a bit different in each language.

I agree. It seems like the Swift users frequently conflate Swift errors with other languages' exception semantics. Using XCTAssertThrows for both Swift errors and Objective-C exceptions muddles the distinction between the two.

ArtSabintsev · 2017-01-14T01:18:16Z

@briancroom @modocache Thanks for that information. Makes a lot more sense. The naming is quite unfortunate and confusing at times. I will make PRs for you this weekend.

modocache · 2017-01-14T03:11:24Z

Oh, just to be clear, that's just my opinion on the naming. Actually changing them would mean a source-breaking change for XCTest users, which I think would be unacceptable. So if you were planning on sending pull requests to change the names, then I think those probably couldn't be merged. Other pull requests are definitely welcome, though! And thanks again for this addition!

ArtSabintsev · 2017-01-14T03:25:15Z

No plans to change names for the exact reason you mentioned!

ArtSabintsev added 5 commits January 13, 2017 00:08

First pass at implementing XCTAssertNoThrow

e5e8eb7

Now outputting caughtError

93bc181

Implemented assertionType in failure cases

f329b3b

Fixed smal bug with how expression was called

7d726f2

Adjusted _XCTRegisterFailure conditions. Added validation test for XC…

31b96ad

…TAssertNoThrow

CodaFi requested changes Jan 13, 2017

View reviewed changes

slavapestov assigned CodaFi Jan 13, 2017

CodaFi requested changes Jan 13, 2017

View reviewed changes

ArtSabintsev added 2 commits January 13, 2017 01:18

Fixed indentation

13094af

Simplified throwable function evaluation and success case requirements.

b6d36fa

CodaFi requested changes Jan 13, 2017

View reviewed changes

Fixed indentation.

5f9352c

CodaFi requested changes Jan 13, 2017

View reviewed changes

Changed success condition

ec8e6c5

All declarations in XCTest in SDK and XCTest in validations-tests hav…

43a7b31

…e 2 spaces instead of 4

Revert "All declarations in XCTest in SDK and XCTest in validations-t…

7410565

…ests have 2 spaces instead of 4" This reverts commit 43a7b31.

Fixes validation test by removing non-existant closure

12ef5bb

Changed constant to _XCTAssertionType.noThrow

5008ed8

CodaFi approved these changes Jan 13, 2017

View reviewed changes

CodaFi merged commit cef4e66 into swiftlang:master Jan 13, 2017

ArtSabintsev deleted the feature/XCTAssertNoThrow branch January 13, 2017 13:19

ArtSabintsev mentioned this pull request Jan 16, 2017

[53] Issue #53 - January 19, 2017 SwiftWeekly/swiftweekly.github.io#165

Closed

Implementation for XCTAssertNoThrow #6776

Implementation for XCTAssertNoThrow #6776

Uh oh!

Conversation

ArtSabintsev commented Jan 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CodaFi commented Jan 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CodaFi Jan 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CodaFi commented Jan 13, 2017

Uh oh!

ArtSabintsev commented Jan 13, 2017

Uh oh!

ArtSabintsev commented Jan 13, 2017

Uh oh!

CodaFi Jan 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CodaFi Jan 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CodaFi commented Jan 13, 2017

Uh oh!

ArtSabintsev commented Jan 13, 2017

Uh oh!

CodaFi commented Jan 13, 2017

Uh oh!

ArtSabintsev commented Jan 13, 2017

Uh oh!

ArtSabintsev commented Jan 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ArtSabintsev commented Jan 13, 2017

Uh oh!

CodaFi commented Jan 13, 2017

Uh oh!

CodaFi commented Jan 13, 2017

Uh oh!

ArtSabintsev commented Jan 13, 2017

Uh oh!

CodaFi commented Jan 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ArtSabintsev commented Jan 13, 2017

Uh oh!

ArtSabintsev commented Jan 13, 2017

Uh oh!

ArtSabintsev commented Jan 13, 2017

Uh oh!

CodaFi commented Jan 13, 2017

Uh oh!

ArtSabintsev commented Jan 13, 2017

ArtSabintsev commented Jan 13, 2017 •

edited

Loading

CodaFi Jan 13, 2017 •

edited

Loading

CodaFi Jan 13, 2017 •

edited

Loading

CodaFi Jan 13, 2017 •

edited

Loading

ArtSabintsev commented Jan 13, 2017 •

edited

Loading

CodaFi commented Jan 13, 2017 •

edited

Loading