Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Update and add test cases to ConvertSubCommandSourceRepositoryTests
Updated testSourceRepositoryAllArgumentsSpecified to use a valid temp directory for it's checkout path argument
because SourceRepository init now guards against invalid checkout paths.

Added a test to make sure SourceRepository guards against invalid checkout paths
  • Loading branch information
Anthony-Eid committed May 31, 2024
commit 923e3281e1a1ff7e6f4315b02acfd100fe45a986
8 changes: 3 additions & 5 deletions Sources/SwiftDocC/SourceRepository/SourceRepository.swift
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a significant amount of whitespace-only changes in all these files which adds noise to the PR. Please remove all those.

Original file line number Diff line number Diff line change
Expand Up @@ -33,14 +33,12 @@ public struct SourceRepository {
formatLineNumber: @escaping (Int) -> String
) {


// guard FileManager.default.directoryExists(atPath: checkoutPath) else {
// throw ValidationError("User provided checkout-path argument {checkoutPath} is invalid.")
// }
// Get the absolute path of a file without the file:// prefix because this function used to only
// expect absolute paths from a user and didn't convert checkoutPath to a URL and back.
let absoluteCheckoutPath = URL(fileURLWithPath: checkoutPath).absoluteString
let startIndex = absoluteCheckoutPath.index(absoluteCheckoutPath.startIndex, offsetBy: 7)

self.checkoutPath = String(absoluteCheckoutPath[startIndex...])

self.sourceServiceBaseURL = sourceServiceBaseURL
self.formatLineNumber = formatLineNumber
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,15 @@ import SwiftDocC
/// Command-line arguments for specifying the catalog's source repository information.
public struct SourceRepositoryArguments: ParsableArguments {
public init() {}

/// The root path on disk of the repository's checkout.
@Option(
help: ArgumentHelp(
"The root path on disk of the repository's checkout."
)
)
public var checkoutPath: String?

/// The source code service used to host the project's sources.
///
/// Required when using `--source-service-base-url`. Supported values are `github`, `gitlab`, and `bitbucket`.
Expand All @@ -36,7 +36,7 @@ public struct SourceRepositoryArguments: ParsableArguments {
)
)
public var sourceService: String?

/// The base URL where the source service hosts the project's sources.
///
/// Required when using `--source-service`. For example, `https://github.com/my-org/my-repo/blob/main`.
Expand Down Expand Up @@ -81,7 +81,10 @@ extension SourceRepository {
guard let sourceServiceBaseURL = URL(string: sourceServiceBaseURL), sourceServiceBaseURL.scheme != nil, sourceServiceBaseURL.host != nil else {
throw ValidationError("Invalid URL '\(sourceServiceBaseURL)' for '--source-service-base-url' argument.")
}





switch sourceService.lowercased() {
case "github":
self = .github(checkoutPath: checkoutPath, sourceServiceBaseURL: sourceServiceBaseURL)
Expand All @@ -94,6 +97,10 @@ extension SourceRepository {
"Unsupported source service '\(sourceService)'. Use 'github', 'gitlab', or 'bitbucket'."
)
}

guard FileManager.default.directoryExists(atPath: checkoutPath) else {
throw ValidationError("Checkout path directory '\(checkoutPath)' doesn't exist for --checkout-path argument.")
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -28,18 +28,22 @@ class ConvertSubcommandSourceRepositoryTests: XCTestCase {
)!

func testSourceRepositoryAllArgumentsSpecified() throws {
let checkoutPath = "checkout path"
var absoluteCheckoutPath = URL(fileURLWithPath: checkoutPath).absoluteString
let startIndex = absoluteCheckoutPath.index(absoluteCheckoutPath.startIndex, offsetBy: 7)
absoluteCheckoutPath = String(absoluteCheckoutPath[startIndex...])
let tempDir = try createTemporaryDirectory(pathComponents: "addFileToCreateValidDirectory")

// removing file:// prefix from checkout path because the directory is not
// recognized as a valid directory with it
let checkoutPath = tempDir.absoluteString
let startIndex = checkoutPath.index(checkoutPath.startIndex, offsetBy: 7)
let absoluteCheckoutPath = String(checkoutPath[startIndex...])


for sourceService in ["github", "gitlab", "bitbucket"] {
try assertSourceRepositoryArguments(
checkoutPath: checkoutPath,
checkoutPath: absoluteCheckoutPath,
sourceService: sourceService,
sourceServiceBaseURL: "https://example.com/path/to/base"
) { action in
XCTAssertEqual(action.sourceRepository?.checkoutPath, absoluteCheckoutPath)
XCTAssertEqual(action.sourceRepository?.checkoutPath, "\(absoluteCheckoutPath)/")
XCTAssertEqual(action.sourceRepository?.sourceServiceBaseURL.absoluteString, "https://example.com/path/to/base")
}
}
Expand All @@ -54,7 +58,29 @@ class ConvertSubcommandSourceRepositoryTests: XCTestCase {
XCTAssertNil(action.sourceRepository)
}
}


func testThrowsValidationErrorWhenCheckoutPathIsInvalid() throws {
let tempDir = try createTemporaryDirectory(named: "tmp").appendingPathComponent("InvalidDirectory", isDirectory: false)
let absoluteCheckoutPath = tempDir.absoluteString
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand correctly, this test if verifying that using a file:// prefix for the command line argument raises an error. AFAIK we don't do that for any other path argument and I don't think it's all that likely that developers will do this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That test is checking that an error is thrown when a path is used as an argument, but the directories of the path don't exist.

Specifically, it's testing this guard I added in SourceRepositoryArguments.swift

guard FileManager.default.directoryExists(atPath: checkoutPath) else {
    throw ValidationError("Checkout path directory '\(checkoutPath)' doesn't exist for --checkout-path argument.")
}

To the end of the SourceRepository.init?(from arguments: SourceRepositoryArguments) function


for sourceService in ["github", "gitlab", "bitbucket"] {
XCTAssertThrowsError(
try assertSourceRepositoryArguments(
checkoutPath: absoluteCheckoutPath,
sourceService: sourceService,
sourceServiceBaseURL: "https://example.com/path/to/base"
)
) { error in
XCTAssertEqual(
(error as? ValidationError)?.message,
"""
Checkout path directory '\(absoluteCheckoutPath)' doesn't exist for --checkout-path argument.
"""
)
}
}
}

func testThrowsValidationErrorWhenSourceServiceIsSpecifiedButNotSourceServiceBaseURL() throws {
XCTAssertThrowsError(
try assertSourceRepositoryArguments(
Expand Down