improving failure output per SR-3275 #921
Conversation
heckj
commented
Jan 26, 2017
•
heckj
commented
- resolves https://bugs.swift.org/browse/SR-3275
- handing through process result utf8output to error for more informative string result
- adds functional test fixture that will intentionally fail
- adds test to validate error from underlying git is propogated on failure
- adds Error capture for tool commands to handle pretty-print of output
| @@ -208,7 +209,7 @@ public class GitRepository: Repository, WorkingCheckout { | |||
| /// The name of the object. | |||
| let name: String | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Please try and leave spurious whitespace changes out of PRs
There was a problem hiding this comment.
Yeah - sorry about that - didn't intend the whitespace changes, but did a quick save & edit with VM and it cleans extra whitespace from lines from previous settings. I'll undo that with a future update.
|
I tried fiddling with The "missing git" scenario: And the "git exists, but failed for some reason" scenario I wasn't sure how thorough to be with these variant error messages and validating them, so the example in place uses a faked-up git command (per other examples in that functional test suite) and validates that the error on STDOUT appears in the process output. Is this a reasonable test setup? I think it at least illustrates that more error information is being presented. There's left-over nuggets in there to clean up before it's ready to merge, but I wanted to expose the WIP for feedback. |
|
We could do what llbuild does, try to lookup the executable. If we don't have one we can throw before even trying to spawn. |
|
@aciidb0mb3r I was thinking of something along those lines as well - something like that was my original thought for what to do to solve this bug, but then I also thought that getting the error from Git (if one happened) reflected out was also valuable per @ddunbar suggestion in the bug report (https://bugs.swift.org/browse/SR-3275). I'm happy to extend this (and testing) to accomodate that use case. Is there thought or a desire to support other command-line programs as pre-requisites? (I.e. |
|
We should do both:
These are orthogonal to each other and this PR looks almost complete. Thanks! |
| try localFileSystem.writeFileContents(fakeGit, bytes: stream.bytes) | ||
|
|
||
| // Make it executable. | ||
| _ = try Process.popen(args: "chmod", "+x", fakeGit.asString) |
There was a problem hiding this comment.
This is fine but we can make an utility method in TestSupport to create executable scripts. Something like:
createExectutableScript(at: fakeGit) { stream in
stream <<< "..."
}There was a problem hiding this comment.
Cool - I'll give that a shot and refactor these methods to use it.
| if let oldPath = oldPath { | ||
| env["PATH"] = env["PATH"]! + ":" + oldPath | ||
| } | ||
| // print("Running with ENV[PATH] = \(String(describing: env["PATH"]))") |
There was a problem hiding this comment.
leftover from learning/debugging and will clean
| func testReportingErrorFromGitCommand() throws { | ||
| fixture(name: "Miscellaneous/MissingDependency") { prefix in | ||
|
|
||
| let fakeGit = prefix.appending(components: "bin", "fakegit") |
There was a problem hiding this comment.
This should be git and not fakegit in string
| } | ||
| // print("Running with ENV[PATH] = \(String(describing: env["PATH"]))") | ||
|
|
||
| // ?? is it acceptable to use the ENV var override for of 'SWIFT_GIT' to make it seem like one doesn't exist to simulate lack of git installed? |
There was a problem hiding this comment.
We can get away by naming the executable as git, at least for now.
There was a problem hiding this comment.
I'll switch it back to git and remove the ENV var override pieces
|
|
||
| let result = try process.waitUntilExit() | ||
| let resultOutputString = try result.utf8Output() | ||
| // print("FAILED PROCESS RESULT: \(resultOutputString)") |
There was a problem hiding this comment.
will clean the commented bits
| // Verify our fake git's error output appears in the process results | ||
| XCTAssert(resultOutputString.contains("Some Error Reported Here"), "Error from fake git was not propogated to process output") | ||
|
|
||
| XCTAssertFalse(try Process.running(process.processID)) |
| let result = try process.waitUntilExit() | ||
| let resultOutputString = try result.utf8Output() | ||
| // print("FAILED PROCESS RESULT: \(resultOutputString)") | ||
| // We should not have exited with 555 from our fake "git". |
There was a problem hiding this comment.
Did you mean to write we should?
| @@ -16,13 +16,13 @@ import func POSIX.getenv | |||
| import enum POSIX.Error | |||
|
|
|||
| enum GitRepositoryProviderError: Swift.Error { | |||
| case gitCloneFailure(url: String, path: AbsolutePath) | |||
| case gitCloneFailure(url: String, path: AbsolutePath, errorOutput: String) | |||
There was a problem hiding this comment.
We should catch this error in Commands/Error.swift so that it can be a bit pretty printed
There was a problem hiding this comment.
I was following along with GitRepositoryProviderError already being defined here as an extension, although I'm not entirely clear on what the conventions on for when you update here vs. Commons/Error.swift. Should I move the whole error enum extension and it's description over to Common/Error.swift? (i.e. lines 18 through 28 in this file?)
There was a problem hiding this comment.
No no, I meant we catch and handle this error in Commands/Error.swift so it is printed better for the user. For example this can be caught like this in the handle(error:) method in that file.
...
case GitRepositoryProviderError. gitCloneFailure(let url, _, let output):
print(error: "Failed to clone \(url.asString):\n" + output)
...There was a problem hiding this comment.
There was a problem hiding this comment.
Ok - I think I get it - I see the errors being handled, and this one wasn't handled there previously it seems. Is the convention to keep the relevant specific errors with the code they relate to, but handle printing the output in Common/Error.swift? To be honest, I hadn't traced where the returned description: String was used. While was looking at the other errors, I noticed there is a .gitCloneFailure defined in Get/Error.swift (seperate from the specific GitRepositoryProviderError that I was poking at originally. It doesn't expose this additional detail - does that need to be extended as well?
There was a problem hiding this comment.
Think of it as API and clients, this module provides an API and Commands is a client which uses this API (and handles that error).
Command is basically a high level client which contains the cli tools: swift-{build, test, package}
You can ignore Get, it's dead code now.
Sources/Commands/Error.swift
Outdated
| @@ -105,6 +107,11 @@ public func handle(error: Any) -> Never { | |||
|
|
|||
| case PackageToolOperationError.insufficientOptions(let usage): | |||
| print(usage, to: &stderr) | |||
|
|
|||
| case GitRepositoryProviderError.gitCloneFailure(let url, let path, let errorOutput): | |||
| //should I be doing something to utilize the gitCloneFailure CustomStringConvertible description here? | |||
There was a problem hiding this comment.
We can remove the CustomStringConvertible conformance
Sources/Commands/Error.swift
Outdated
|
|
||
| case GitRepositoryProviderError.gitCloneFailure(let url, let path, let errorOutput): | ||
| //should I be doing something to utilize the gitCloneFailure CustomStringConvertible description here? | ||
| print(error: "Failed to clone \(url) to \(path): \(errorOutput)") |
There was a problem hiding this comment.
use path.asString
Add \n after colon
|
Once I've added the "checking for git" prior to invoking it mechanism, I'll also rebase this down... |
| } | ||
| //extension GitRepositoryProviderError: CustomStringConvertible { |
There was a problem hiding this comment.
Please remove the commented code
|
@swift-ci please test |
1 similar comment
|
@swift-ci please test |
