Skip to content
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.

Sign up for GitHub

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

Introducing Timer.record(_ duration:), closes #114 #133

Merged
merged 11 commits into from
Jun 28, 2023
Merged

Introducing Timer.record(_ duration:), closes #114 #133

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
37ae7aa
Adding Timer.record(_ duration: Duration)
natikgadzhi Jun 23, 2023
e0ce4e8
Wrap Duration code in Swift 5.7 checks
natikgadzhi Jun 23, 2023
d0bd9aa
Fix formatting in tests
natikgadzhi Jun 23, 2023
9cccde4
scripts/sounness check if swiftformat is installed
natikgadzhi Jun 23, 2023
b019460
Review feedback: recordin in nanoseconds
natikgadzhi Jun 24, 2023
02fc732
Timer.record(_ duration:): protect from overflows
natikgadzhi Jun 27, 2023
f8316f2
Make Timer.record(_ duration: Duration) record Int64.max on overflow
natikgadzhi Jun 28, 2023
9b835a7
Timer.record(_ duration:) review feedback cleanup
natikgadzhi Jun 28, 2023
fd5b5d2
Timer.record(_ duration:) review feedback: XCTSkip on Swift version
natikgadzhi Jun 28, 2023
84db69d
Timer.record(_ duration:) review feedback: formatting
natikgadzhi Jun 28, 2023
99ec049
Timer.record(_ duration:) review feedback: only use XCTSkip on Swift …
natikgadzhi Jun 28, 2023
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
5 changes: 0 additions & 5 deletions Sources/Metrics/Metrics.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,11 +76,6 @@ extension Timer {
}

#if swift(>=5.7)

public enum TimerError: Error {
case durationToIntOverflow
}

extension Timer {
/// Convenience for recording a duration based on ``Duration``.
///
natikgadzhi marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
2 changes: 1 addition & 1 deletion Tests/MetricsTests/MetricsTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ class MetricsExtensionsTests: XCTestCase {
// tests on Linux in MetricsTests+XCTest don't complain that the func does not exist.
#if swift(>=5.7)
guard #available(iOS 16, macOS 13, tvOS 15, watchOS 8, *) else {
return
throw XCTSkip("Timer.record(_ duration: Duration) is available on Swift 5.7+")
Copy link
Member

@tomerd tomerd Jun 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

do the same for earlier versions of Swift? this one is for the availability.

Copy link
Contributor Author

@natikgadzhi natikgadzhi Jun 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Tried that — #available doesn't like to be treated as an expression and only works with guard and if, seemingly. Didn't try for the language check, though. Let me see if that works.

UPD: I think wrapping the check in a closure before feeding it to XCSkipIf works. Might look ugly, trying it out now.

Copy link
Member

Choose a reason for hiding this comment

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

adding an #else does not work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

try! XCTSkipIf({
    #if swift(<5.7)
        return true
    #else
        if #available(iOS 16, macOS 13, tvOS 15, watchOS 8, *) {
            return false
        } else {
            return true
        }
    #endif
}(), "Timer.record(_ duration: Duration) is available on Swift 5.7+")

That works! But mixing #if and regular if looks weird to me. Perhaps two separate for #available and #if swift are okay?

#if swift(<5.7)
    throw XCTSkip("Timer.record(_ duration: Duration) is not available on this version of Swift")
#endif

guard #available(iOS 16, macOS 13, tvOS 15, watchOS 8, *) else {
    throw XCTSkip("Timer.record(_ duration: Duration) is not available on this version of Swift")
}

Looks more readable? The closure with nested ifs and explicit returns seems too verbose.

Should I stick with two separate checks?

Copy link
Member

Choose a reason for hiding this comment

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

I think we will have to #if swift(>=5.7) around the entire body of the test as done now since it used types that are not available in previous versions (build will fail) but we can add an #else to throw XCTSkip there

Copy link
Member

Choose a reason for hiding this comment

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

note tested, but...

func testTimerDuration() throws {
        // Wrapping only the insides of the test case so that the generated
        // tests on Linux in MetricsTests+XCTest don't complain that the func does not exist.
        #if swift(>=5.7)
        guard #available(iOS 16, macOS 13, tvOS 15, watchOS 8, *) else {
            throw XCTSkip("Timer.record(_ duration: Duration) is available on Swift 5.7+")
        }

        let metrics = TestMetrics()
        MetricsSystem.bootstrapInternal(metrics)

        let name = "timer-\(UUID().uuidString)"
        let timer = Timer(label: name)

        let duration = Duration(secondsComponent: 3, attosecondsComponent: 123_000_000_000_000_000)
        let nanoseconds = duration.components.seconds * 1_000_000_000 + duration.components.attoseconds / 1_000_000_000
        timer.record(duration)

        // Record a Duration that would overflow,
        // expect Int64.max to be recorded.
        timer.record(Duration(secondsComponent: 10_000_000_000, attosecondsComponent: 123))

        let testTimer = try metrics.expectTimer(timer)
        XCTAssertEqual(testTimer.values.count, 2, "expected number of entries to match")
        XCTAssertEqual(testTimer.values.first, nanoseconds, "expected value to match")
        XCTAssertEqual(testTimer.values[1], Int64.max, "expected to record Int64.max if Durataion overflows")
        XCTAssertEqual(metrics.timers.count, 1, "timer should have been stored")
        #else
        throw XCTSkip("Timer.record(_ duration: Duration) is available on Swift 5.7+")
        #endif
    }

Copy link
Contributor Author

@natikgadzhi natikgadzhi Jun 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Alright! Pushed up.

UPD wait a sec, broke CI. XCTSkip is not available on Swift 5.0 and 5.1 🤦🏼

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay,

#if swift(>=5.7)
// ... 
#elseif swift(>=5.2)
throw XCTSkip("Timer.record(_ duration: Duration) is only available on Swift >=5.7")
#endif

I think that'd be good enough, Swift 5.1 is not that common anymore anyway?

}

let metrics = TestMetrics()
Expand Down