-
Notifications
You must be signed in to change notification settings - Fork 83
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Add PerformanceTests #1910
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments mostly around naming & organization.
PerformanceTests/Sources/PerformanceTestScript/Util/Benchmark.swift
Outdated
Show resolved
Hide resolved
PerformanceTests/Sources/PerformanceTestScript/Util/Benchmark.swift
Outdated
Show resolved
Hide resolved
PerformanceTests/Sources/PerformanceTestScript/Util/Benchmark.swift
Outdated
Show resolved
Hide resolved
PerformanceTests/Sources/PerformanceTestScript/Util/Benchmark.swift
Outdated
Show resolved
Hide resolved
PerformanceTests/Sources/PerformanceTestScript/PerformanceTestRun.swift
Outdated
Show resolved
Hide resolved
PerformanceTests/Sources/PerformanceTestScript/Util/Benchmark.swift
Outdated
Show resolved
Hide resolved
PerformanceTests/Sources/PerformanceTestRunner/Core/Model/Dimension.swift
Show resolved
Hide resolved
enum OperatingSystem: String { | ||
case macOS = "macOS" | ||
case Linux = "Linux" | ||
case Unknown = "Unknown" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: cases of a Swift enum should start with lower case (macOS
, linux
, unknown
)
|
||
/// Retrieves the SDK version from a file. | ||
static func getSdkVersion() -> String { | ||
return ProcessRunner.runProcess(arguments: ["cat", "../../aws-sdk-swift/Package.version"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we do it elsewhere, but for safety maybe trim leading & trailing whitespace before returning version string
PerformanceTests/Sources/PerformanceTestRunner/Core/Runners/PerformanceTestRunner.swift
Show resolved
Hide resolved
|
||
struct Dimension: Codable { | ||
let name: String // The name of the dimension. MUST comply with CloudWatch constraints on dimension name. | ||
let value: String // The value of the dimension. MUST comply with CloudWatch constraints on dimension value. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These may as well be doc comments (three slashes on line before property) so that they show in tooltip help
Also, the same sort of comments are appropriate on PerformanceTestReport
and PerformanceTestResult
which also form the test report structure.
Issue #
SWIFT-2636
Description of changes
swift run
XCTest
was considered but instead this is an executable for fine grained control over the outputExample Output:
New/existing dependencies impact assessment, if applicable
Conventional Commits
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.