Merge pull request #5 from jamf/story/SUS-7308-add-output-to-errors

SUS-7308 added some error support for interop with Objc

CHANGELOG.md

@@ -0,0 +1,33 @@
+## Subprocess
+
+Subprocess is a Swift library for macOS providing interfaces for both synchronous and asynchronous process execution. 
+
+All notable changes to this project will be documented in this file.
+
+The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
+and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
+
+## [Unreleased]
+
+### Changed
+- Breaking: added the output of the command to Shell's exception exitedWithNonZeroStatus error to better conform to objc interop and NSError
+
+
+## 1.1.0 - 2020-05-15
+
+### Added
+- Added dynamic library targets to SPM
+
+### Fixed
+- Fixed naming convention to match Jamf internal convention
+
+## 1.0.1 - 2020-03-13
+
+### Added
+- Added Cocoapods support
+
+
+## 1.0.0 - 2020-03-13
+
+### Added
+- All support for the initial release 

Sources/Subprocess/Errors.swift

@@ -31,7 +31,8 @@ import Foundation
 public enum SubprocessError: Error {
 
     /// The process completed with a non-zero exit code
-    case exitedWithNonZeroStatus(Int32)
+    /// and a custom error message.
+    case exitedWithNonZeroStatus(Int32, String)
 
     /// The property list object could not be cast to expected type
     case unexpectedPropertyListObject(String)
@@ -45,3 +46,36 @@ public enum SubprocessError: Error {
     /// Output string could not be encoded
     case outputStringEncodingError
 }
+
+extension SubprocessError: LocalizedError {
+    public var errorDescription: String? {
+        switch self {
+        case .exitedWithNonZeroStatus(_, let errorMessage):
+            return "\(errorMessage)"
+        case .unexpectedPropertyListObject(_):
+            // Ignoring the plist contents parameter as we don't want that in the error message
+            return "The property list object could not be cast to expected type"
+        case .unexpectedJSONObject(_):
+            // Ignoring the json contents parameter as we don't want that in the error message
+            return "The JSON object could not be cast to expected type"
+        case .inputStringEncodingError:
+            return "Input string could not be encoded"
+        case .outputStringEncodingError:
+            return "Output string could not be encoded"
+        }
+    }
+}
+
+/// Common NSError methods for better interop with Objective-C
+extension SubprocessError: CustomNSError {
+    public var errorCode: Int {
+        switch self {
+        case .exitedWithNonZeroStatus(let errorCode, _):
+            return Int(errorCode)
+        case .unexpectedPropertyListObject: return 10
+        case .unexpectedJSONObject: return 20
+        case .inputStringEncodingError: return 30
+        case .outputStringEncodingError: return 40
+        }
+    }
+}

Sources/Subprocess/Info.plist

@@ -15,7 +15,7 @@
 	<key>CFBundlePackageType</key>
 	<string>$(PRODUCT_BUNDLE_PACKAGE_TYPE)</string>
 	<key>CFBundleShortVersionString</key>
-	<string>1.0.1</string>
+	<string>2.0.0</string>
 	<key>CFBundleVersion</key>
 	<string>$(CURRENT_PROJECT_VERSION)</string>
 </dict>

Sources/Subprocess/Shell.swift

@@ -66,12 +66,17 @@ public class Shell {
     public func exec<T>(input: Input? = nil,
                         options: OutputOptions = .stdout,
                         transformBlock: (_ process: Subprocess, _ data: Data) throws -> T) throws -> T {
-        var buffer = Data()
+        var stdoutBuffer = Data()
+        var stderrBuffer = Data()
         try process.launch(input: input,
-                           outputHandler: options.contains(.stdout) ? { buffer.append($0) } : nil,
-                           errorHandler: options.contains(.stderr) ? { buffer.append($0) } : nil)
+                           outputHandler: options.contains(.stdout) ? { stdoutBuffer.append($0) } : nil,
+                           errorHandler: options.contains(.stderr) ? { stderrBuffer.append($0) } : nil)
         process.waitForTermination()
-        return try transformBlock(process, buffer)
+        // doing this so we can consistently get stdout before stderr when using the combined option
+        var combinedBuffer = Data()
+        combinedBuffer.append(stdoutBuffer)
+        combinedBuffer.append(stderrBuffer)
+        return try transformBlock(process, combinedBuffer)
     }
 
     /// Executes shell command expecting exit code of zero and returning the output data
@@ -84,7 +89,10 @@ public class Shell {
     public func exec(input: Input? = nil, options: OutputOptions = .stdout) throws -> Data {
         return try exec(input: input, options: options) { process, data in
             let exitCode = process.exitCode
-            guard exitCode == 0 else { throw SubprocessError.exitedWithNonZeroStatus(exitCode) }
+            guard exitCode == 0 else {
+                let message = String(data: data, encoding: .utf8)
+                throw SubprocessError.exitedWithNonZeroStatus(exitCode, message ?? "")
+            }
             return data
         }
     }
@@ -124,7 +132,9 @@ public class Shell {
                      encoding: String.Encoding) throws -> String {
         return try exec(input: input, options: options, encoding: encoding) { process, text in
             let exitCode = process.exitCode
-            guard exitCode == 0 else { throw SubprocessError.exitedWithNonZeroStatus(exitCode) }
+            guard exitCode == 0 else {
+                throw SubprocessError.exitedWithNonZeroStatus(exitCode, text)
+            }
             return text
         }
     }

Sources/SubprocessMocks/Info.plist

@@ -15,7 +15,7 @@
 	<key>CFBundlePackageType</key>
 	<string>$(PRODUCT_BUNDLE_PACKAGE_TYPE)</string>
 	<key>CFBundleShortVersionString</key>
-	<string>1.0.1</string>
+	<string>2.0.0</string>
 	<key>CFBundleVersion</key>
 	<string>$(CURRENT_PROJECT_VERSION)</string>
 </dict>

Subprocess.podspec

@@ -1,6 +1,6 @@
 Pod::Spec.new do |s|
   s.name         = 'Subprocess'
-  s.version      = '1.0.1'
+  s.version      = '2.0.0'
   s.summary      = 'Wrapper for NSTask used for running processes and shell commands on macOS.'
   s.license      = { :type => 'MIT', :text => "" }
   s.description  = <<-DESC

Subprocess.xcodeproj/project.pbxproj

@@ -20,7 +20,7 @@
 		6EDDC6DA241037F500E171C6 /* MockSubprocessDependencyBuilder.swift in Sources */ = {isa = PBXBuildFile; fileRef = 6EDDC6AF2410378100E171C6 /* MockSubprocessDependencyBuilder.swift */; };
 		6EDDC6DB241037F500E171C6 /* MockProcess.swift in Sources */ = {isa = PBXBuildFile; fileRef = 6EDDC6B12410378100E171C6 /* MockProcess.swift */; };
 		6EDDC6E52410391500E171C6 /* Subprocess.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = 6EDDC6382410369400E171C6 /* Subprocess.framework */; };
-		6EDDC6F92410395800E171C6 /* ShellTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 6EDDC6B82410378100E171C6 /* ShellTests.swift */; };
+		6EDDC6F92410395800E171C6 /* ShellSystemTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 6EDDC6B82410378100E171C6 /* ShellSystemTests.swift */; };
 		6EDDC6FB2410396000E171C6 /* SubprocessTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 6EDDC6BC2410378100E171C6 /* SubprocessTests.swift */; };
 		6EDDC6FC2410396000E171C6 /* ShellTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 6EDDC6BD2410378100E171C6 /* ShellTests.swift */; };
 		6EDDC6FD24105F8300E171C6 /* SubprocessMocks.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = 6EDDC6CB241037CE00E171C6 /* SubprocessMocks.framework */; };
@@ -61,7 +61,7 @@
 		6EDDC6B02410378100E171C6 /* Info.plist */ = {isa = PBXFileReference; lastKnownFileType = text.plist.xml; path = Info.plist; sourceTree = "<group>"; };
 		6EDDC6B12410378100E171C6 /* MockProcess.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = MockProcess.swift; sourceTree = "<group>"; };
 		6EDDC6B22410378100E171C6 /* Subprocess.podspec */ = {isa = PBXFileReference; lastKnownFileType = text; path = Subprocess.podspec; sourceTree = "<group>"; };
-		6EDDC6B82410378100E171C6 /* ShellTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = ShellTests.swift; sourceTree = "<group>"; };
+		6EDDC6B82410378100E171C6 /* ShellSystemTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = ShellSystemTests.swift; sourceTree = "<group>"; };
 		6EDDC6B92410378100E171C6 /* Info.plist */ = {isa = PBXFileReference; lastKnownFileType = text.plist.xml; path = Info.plist; sourceTree = "<group>"; };
 		6EDDC6BC2410378100E171C6 /* SubprocessTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SubprocessTests.swift; sourceTree = "<group>"; };
 		6EDDC6BD2410378100E171C6 /* ShellTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = ShellTests.swift; sourceTree = "<group>"; };
@@ -182,7 +182,7 @@
 		6EDDC6B52410378100E171C6 /* SystemTests */ = {
 			isa = PBXGroup;
 			children = (
-				6EDDC6B82410378100E171C6 /* ShellTests.swift */,
+				6EDDC6B82410378100E171C6 /* ShellSystemTests.swift */,
 				6EDDC6B92410378100E171C6 /* Info.plist */,
 			);
 			path = SystemTests;
@@ -452,7 +452,7 @@
 			isa = PBXSourcesBuildPhase;
 			buildActionMask = 2147483647;
 			files = (
-				6EDDC6F92410395800E171C6 /* ShellTests.swift in Sources */,
+				6EDDC6F92410395800E171C6 /* ShellSystemTests.swift in Sources */,
 			);
 			runOnlyForDeploymentPostprocessing = 0;
 		};

Tests/SystemTests/ShellTests.swift → Tests/SystemTests/ShellSystemTests.swift

@@ -1,7 +1,7 @@
 import XCTest
 @testable import Subprocess
 
-final class ShellTests: XCTestCase {
+final class ShellSystemTests: XCTestCase {
 
     override func setUp() {
         SubprocessDependencyBuilder.shared = SubprocessDependencyBuilder()

Tests/UnitTests/ShellTests.swift

@@ -28,14 +28,21 @@ final class ShellTests: XCTestCase {
     func testExecReturningDataWhenExitCodeIsNoneZero() {
         // Given
         let exitCode = Int32.random(in: 1...Int32.max)
-        Shell.expect(command, input: nil, standardError: Data(), exitCode: exitCode)
+        let stdoutData = "stdout example".data(using: .utf8)
+        let stderrData = "stderr example".data(using: .utf8)
+        Shell.expect(command, input: nil, standardOutput: stdoutData, standardError: stderrData, exitCode: exitCode)
 
         // When
-        XCTAssertThrowsError(_ = try Shell(command).exec()) {
-            switch ($0 as? SubprocessError) {
-            case .exitedWithNonZeroStatus(let status):
+        XCTAssertThrowsError(_ = try Shell(command).exec()) { error in
+            switch (error as? SubprocessError) {
+            case .exitedWithNonZeroStatus(let status, let errorMessage):
                 XCTAssertEqual(status, exitCode)
-            default: XCTFail("Unexpected error type: \($0)")
+                let failMsg = "error message should have contained the results from only stdout but was \(errorMessage)"
+                XCTAssertTrue(errorMessage.contains("stdout example"), failMsg)
+                XCTAssertEqual(errorMessage, error.localizedDescription, "should have also set localizedDescription")
+                let nsError = error as NSError
+                XCTAssertEqual(Int(status), nsError.code, "should have also set the NSError exit code")
+            default: XCTFail("Unexpected error type: \(error)")
             }
         }
 
@@ -92,8 +99,7 @@ final class ShellTests: XCTestCase {
 
         // Then
         let text = String(data: result, encoding: .utf8)
-        XCTAssertTrue(text?.contains(expectedStdout) ?? false)
-        XCTAssertTrue(text?.contains(expectedStderr) ?? false)
+        XCTAssertEqual("\(expectedStdout)\(expectedStderr)", text, "should have combined stdout and stderror")
         Shell.verify { XCTFail($0.message, file: $0.file, line: $0.line) }
     }
 
@@ -102,14 +108,18 @@ final class ShellTests: XCTestCase {
     func testExecReturningStringWhenExitCodeIsNoneZero() {
         // Given
         let exitCode = Int32.random(in: 1...Int32.max)
-        Shell.expect(command, input: nil, stderr: "Some error occured", exitCode: exitCode)
-
+        let stdoutText = "should not show up"
+        let stderrText = "should show up"
+        Shell.expect(command, input: nil, stdout: stdoutText, stderr: stderrText, exitCode: exitCode)
         // When
-        XCTAssertThrowsError(_ = try Shell(command).exec(encoding: .utf8)) {
-            switch ($0 as? SubprocessError) {
-            case .exitedWithNonZeroStatus(let status):
+        XCTAssertThrowsError(_ = try Shell(command).exec(options: .stderr, encoding: .utf8)) { error in
+            switch (error as? SubprocessError) {
+            case .exitedWithNonZeroStatus(let status, let errorMessage):
                 XCTAssertEqual(status, exitCode)
-            default: XCTFail("Unexpected error type: \($0)")
+                let failMsg = "should have put just stderr in the error: \(errorMessage)"
+                XCTAssertEqual("should show up", errorMessage, failMsg)
+                XCTAssertEqual(errorMessage, error.localizedDescription, "also should have set localizedDescription")
+            default: XCTFail("Unexpected error type: \(error)")
             }
         }