Add @discardableResult annotation to applicable public API methods. #166

briancroom · 2016-08-19T21:43:25Z

This maintains consistency with the Darwin XCTest APIs. This also cleans up some warnings in the functional test suite which were being generated because this annotation wasn't present.

I'll note that there's a case to be made that expectation(description:file:line:) should not have @discardableResult, because the expectation it produces can only be fulfilled by invoking a method on the returned value. Nonetheless, I think it's better to keep the APIs in sync for right now and revisit in the future.

This maintains consistency with the Darwin XCTest APIs.

briancroom · 2016-08-19T21:43:31Z

@swift-ci Please test

briancroom · 2016-08-19T23:25:09Z

Although the PR CI did pass, I've noticed a couple of disturbing issues from the OS X CI log. Firstly, it seems that the binaries for the functional tests are failing to launch properly, with the following error:

dyld: Library not loaded: @rpath/libswiftCore.dylib
  Referenced from: /Users/buildnode/jenkins/workspace/swift-corelibs-xctest-PR-osx/Ninja-DebugAssert/xctest-macosx-x86_64/Debug/SwiftXCTest.framework/Versions/A/Frameworks/libswiftDarwin.dylib
  Reason: Incompatible library version: libswiftDarwin.dylib requires version 1.0.0 or later, but libswiftCore.dylib provides version 0.0.0

Secondly, the CI is indicating a pass even though there are failing tests.

modocache · 2016-08-20T03:32:35Z

A passing Linux CI build is probably sufficient for us to merge this pull request. The macOS failures definitely concern me, though. It looks like the macOS build failed for #165 as well, but Swift CI reported it as a success.

@shahmishal, any idea what might be going on here?

briancroom · 2016-08-20T06:46:37Z

I forgot to mention that I filed a couple of bugs for tracking this:
https://bugs.swift.org/plugins/servlet/mobile#issue/SR-2433
https://bugs.swift.org/plugins/servlet/mobile#issue/SR-2434

shahmishal · 2016-08-20T07:27:25Z

This might be related to exit code of xcodebuild with grep -v " export".

xcodebuild -workspace /Users/buildnode/jenkins/workspace/swift-corelibs-xctest-PR-osx/swift-corelibs-xctest/XCTest.xcworkspace -scheme SwiftXCTestFunctionalTests -configuration Debug SWIFT_EXEC="/Users/buildnode/jenkins/workspace/swift-corelibs-xctest-PR-osx/Ninja-DebugAssert/swift-macosx-x86_64/bin/swiftc" SWIFT_LINK_OBJC_RUNTIME=YES SYMROOT="/Users/buildnode/jenkins/workspace/swift-corelibs-xctest-PR-osx/Ninja-DebugAssert/xctest-macosx-x86_64" OBJROOT="/Users/buildnode/jenkins/workspace/swift-corelibs-xctest-PR-osx/Ninja-DebugAssert/xctest-macosx-x86_64" | grep -v "    export"

I am re-building to see if exit code was set correctly.
https://ci.swift.org/job/swift-corelibs-xctest-PR-osx/107/console

shahmishal · 2016-08-20T08:38:13Z

exit code after test failure with grep command is set to "0".

$ xcodebuild -workspace /Users/buildnode/jenkins/workspace/swift-corelibs-xctest-PR-osx/swift-corelibs-xctest/XCTest.xcworkspace -scheme SwiftXCTestFunctionalTests -configuration Debug SWIFT_EXEC="/Users/buildnode/jenkins/workspace/swift-corelibs-xctest-PR-osx/Ninja-DebugAssert/swift-macosx-x86_64/bin/swiftc" SWIFT_LINK_OBJC_RUNTIME=YES SYMROOT="/Users/buildnode/jenkins/workspace/swift-corelibs-xctest-PR-osx/Ninja-DebugAssert/xctest-macosx-x86_64" OBJROOT="/Users/buildnode/jenkins/workspace/swift-corelibs-xctest-PR-osx/Ninja-DebugAssert/xctest-macosx-x86_64" | grep -v "    export"
...
$ echo $?
0

We will need to look into https://github.com/apple/swift-corelibs-xctest/blob/a21a8546fe2e3e2d5b2ed062300982396898f3d1/build_script.py#L113

modocache · 2016-08-20T15:49:55Z

It looks like the grep here is meant to make the output more readable: #97 (comment)

Unfortunately, since I think all builds, regardless of whether they pass, include export, this grep causes all macOS builds to pass right now. I'll submit a pull request to remove it for now, and we can talk about how to make the output more readable as a follow-up.

briancroom · 2016-08-22T18:08:59Z

Just to make sure everything is in order, let's wait and re-run CI here once #167 has gone in, then we can confidently merge this and feel confident that CI is once again providing the feedback we expect from it.

briancroom · 2016-08-22T20:31:53Z

@swift-ci Please test

briancroom · 2016-08-23T00:55:32Z

@swift-ci Please test Linux

briancroom · 2016-08-23T03:38:52Z

@parkera Do you recognize this Foundation test failure from other bots? It seems to be consistently crashing in TestNSOperationQueue.test_OperationPriorities

https://ci.swift.org/job/swift-corelibs-xctest-PR-Linux/114/
https://ci.swift.org/job/swift-corelibs-xctest-PR-Linux/115/
https://ci.swift.org/job/swift-corelibs-xctest-PR-Linux/116/

seabaylea · 2016-08-23T13:49:15Z

@briancroom The failures in TestNSOperationQueue.test_OperationPriorities occur because it assumes that Dispatch has been integrated into the builds - which is has been everywhere else. The problem is that the XCTest PR CI jobs haven't been updated.

The job config needs to become:

swift/utils/build-script -R -T --llbuild --swiftpm --xctest --foundation --libdispatch -- --reconfigure

ie. --libdispatch needs to be added.

@shahmishal Can you update the XCTest PR builds?

modocache · 2016-08-23T14:23:11Z

If the tests fail without the --libdispatch flag, shouldn't build-script-impl add that flag automatically when it is missing? We do something similar already: if you run build-script --xctest, we infer build-script --foundation --xctest, since Foundation is a dependency of XCTest.

seabaylea · 2016-08-23T15:43:20Z

Good suggestion - I'll take the same approach for Dispatch.

seabaylea · 2016-08-23T16:08:17Z

Ok - created the following to update the build scripts to infer --libdispatch if building Foundation:
swiftlang/swift#4458

briancroom · 2016-08-24T14:58:08Z

With #169 and swiftlang/swift#4458 merged, let's see if this will pass now!

briancroom · 2016-08-24T14:58:14Z

@swift-ci Please test Linux

modocache · 2016-08-24T16:28:34Z

The failure in Foundation is unrelated to this change:

TestFoundation/TestNSURLSession.swift:70: error: TestURLSession.test_dataTaskWithURLCompletionHandler : XCTAssertEqual failed: ("Washington D.C.") is not equal to ("Washington, D.C.") - Did not receive expected value

The test is missing a comma. Apparently it calls out to an external resource and that external website has changed its response to now include a comma. /cc @pushkarnk, who added the test.

In any case, I think this pull request is good to merge, @briancroom. I'll follow up about the Foundation test.

modocache · 2016-08-24T16:31:09Z

Oops! You beat me to it, haha. Sorry for the noise! 😛

briancroom · 2016-08-24T16:31:32Z

It looks like Foundation has an outstanding PR for this failure already: swiftlang/swift-corelibs-foundation#590

I think it's worth keeping this open a little bit longer so we can keep pinging @swift-ci on it until we're all green here, since there has been so much CI churn recently :)

shahmishal · 2016-08-24T18:26:47Z

@swift-ci Please test Linux

shahmishal · 2016-08-24T18:45:43Z

@seabaylea XCTest PR updated to use --libdispatch.

briancroom · 2016-08-24T19:09:31Z

Excellent! This looks great. Thanks to everyone who pitched in.

Add @discardableResult annotation to applicable public API methods.

3dc89f4

This maintains consistency with the Darwin XCTest APIs.

modocache mentioned this pull request Aug 20, 2016

[build_script] Remove grep since it always passes #167

Merged

modocache mentioned this pull request Aug 23, 2016

Add Dispatch overlay to test configuration #169

Merged

briancroom mentioned this pull request Aug 24, 2016

PR 426 - Fixing a test and removing debug prints swiftlang/swift-corelibs-foundation#590

Merged

briancroom merged commit a5563a3 into swiftlang:master Aug 24, 2016

briancroom deleted the discardable-result branch August 24, 2016 19:09

briancroom mentioned this pull request Dec 13, 2016

add @discardableResult to changeCurrentDirectoryPath(_:) swiftlang/swift-corelibs-foundation#741

Merged

briancroom mentioned this pull request May 9, 2022

[SR-2433] [XCTest] PR CI is malfunctioning on OS X #379

Closed

Add @discardableResult annotation to applicable public API methods. #166

Add @discardableResult annotation to applicable public API methods. #166

Uh oh!

Conversation

briancroom commented Aug 19, 2016

Uh oh!

briancroom commented Aug 19, 2016

Uh oh!

briancroom commented Aug 19, 2016

Uh oh!

modocache commented Aug 20, 2016

Uh oh!

briancroom commented Aug 20, 2016

Uh oh!

shahmishal commented Aug 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shahmishal commented Aug 20, 2016

Uh oh!

modocache commented Aug 20, 2016

Uh oh!

briancroom commented Aug 22, 2016

Uh oh!

briancroom commented Aug 22, 2016

Uh oh!

briancroom commented Aug 23, 2016

Uh oh!

briancroom commented Aug 23, 2016

Uh oh!

seabaylea commented Aug 23, 2016

Uh oh!

modocache commented Aug 23, 2016

Uh oh!

seabaylea commented Aug 23, 2016

Uh oh!

seabaylea commented Aug 23, 2016

Uh oh!

briancroom commented Aug 24, 2016

Uh oh!

briancroom commented Aug 24, 2016

Uh oh!

modocache commented Aug 24, 2016

Uh oh!

modocache commented Aug 24, 2016

Uh oh!

briancroom commented Aug 24, 2016

Uh oh!

shahmishal commented Aug 24, 2016

Uh oh!

shahmishal commented Aug 24, 2016

Uh oh!

briancroom commented Aug 24, 2016

Uh oh!

Uh oh!

shahmishal commented Aug 20, 2016 •

edited

Loading