Skip to content

Speed up directory monitoring tests #1325

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Speed up directory monitoring tests #1325

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
12 changes: 8 additions & 4 deletions Tests/SwiftDocCUtilitiesTests/DirectoryMonitorTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,8 @@ class DirectoryMonitorTests: XCTestCase {
}
}

/// - Warning: Please do not overuse this method as it takes 10s of wait time and can potentially slow down running the test suite.
/// - Warning: Please do not overuse this method as it takes 2 seconds of
/// wait time per invocation, and can potentially slow down the test suite.
private func monitorNoUpdates(url: URL, testBlock: @escaping () throws -> Void, file: StaticString = #filePath, line: UInt = #line) throws {
let monitor = try DirectoryMonitor(root: url) { rootURL, url in
XCTFail("Did produce file update event for a hidden file", file: file, line: line)
Expand All @@ -104,12 +105,15 @@ class DirectoryMonitorTests: XCTestCase {
try? testBlock()
}

// For the test purposes we assume a file change event will be delivered within generous 10 seconds.
DispatchQueue.global().asyncAfter(deadline: .now() + 10) {
// For the test purposes we assume a file change event will be delivered within 1.5 seconds.
// This also aligns with the `monitor` method above, that ensures that file change events
// in tests are received within 1.5 seconds. If this works too eagerly, then the other tests
// in this suite will fail.
DispatchQueue.global().asyncAfter(deadline: .now() + 1.5) {
didNotTriggerUpdateForHiddenFile.fulfill()
}

wait(for: [didNotTriggerUpdateForHiddenFile], timeout: 20)
wait(for: [didNotTriggerUpdateForHiddenFile], timeout: 2)
Copy link
Contributor

@d-ronnqvist d-ronnqvist Oct 31, 2025

Choose a reason for hiding this comment

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

This also doesn't speed up the test as long as it passes. Even if I increase both of these to 200 second testMonitorDoesNotTriggerUpdates only takes ~20 seconds for me to run locally.

Copy link
Contributor Author

@snprajwal snprajwal Nov 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, the speed-up comes from reducing the timeout for .fulfill() a few lines above. Since the fulfillment happens in 1.5s, this wait() will always complete shortly after that duration, irrespective of whether the timeout is 2s or 20s. I reduced it to 2s just so it's clear that it waits slightly longer than the time taken for .fulfill() to run. I'm happy to revert it to 20s if you prefer that, but I don't see how that makes the test any less fragile.

Copy link
Contributor

@d-ronnqvist d-ronnqvist Nov 7, 2025

Choose a reason for hiding this comment

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

I think either is fine. The only subtle thing I like about the 20s timeout—but that reason may not apply generally—is that it's large enough that my mind interprets it as a timeout that won't be waited for in practice. In other words; if I see a 20s timeout I think "this finishes as soon as the expectation is fulfilled" whereas if I see a 2s timeout there's some doubt in my mind that it needs to wait the full two seconds.

Copy link
Contributor

@d-ronnqvist d-ronnqvist Nov 7, 2025

Choose a reason for hiding this comment

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

Taking a second look at this test now I find it to be overly complicated. I feel that this function would be simpler if it creates an expectation that it expected not to be fulfilled. Then we wouldn't need both the asyncAfter and the wait.

let fileUpdateEvent = expectation(description: "Unexpectedly triggered an update event")
fileUpdateEvent.isInverted = true // We don't expect any file updates

let monitor = try DirectoryMonitor(root: url) { rootURL, url in
    fileUpdateEvent.fulfill()
}

try monitor.start()
defer {
    monitor.stop()
}

// For the test purposes we assume a file change event will be delivered within 1.5 seconds.
waitForExpectations(timeout: 1.5)

Copy link
Contributor

@d-ronnqvist d-ronnqvist Nov 7, 2025

Choose a reason for hiding this comment

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

Similarly, the other monitor helper function could be simplified by removing the notifications and waiting for the expectations directly:

let fileUpdatedExpectation = expectedChangeOrigin != nil
    ? expectation(description: "File updated event")
    : nil

let treeRefreshedExpectation = isTreeReloadExpected
    ? expectation(description: "Watched tree was refreshed")
    : nil

// Create a directory monitor and handle its events.
let monitor = try DirectoryMonitor(root: rootURL) { _, url in
    if let expectedChangeOrigin {
        XCTAssertTrue(fileURLsAreEqual(expectedChangeOrigin, url), "'\(expectedChangeOrigin.path)' is not equal to \(url.path)", file: file, line: line)
    }
    fileUpdatedExpectation?.fulfill()
}
monitor.didReloadWatchedDirectoryTree = { _ in
    treeRefreshedExpectation?.fulfill()
}

try monitor.start()
defer {
    monitor.stop()
}

// Run the block that's supposed to create the trigger events in the file system.
try triggerBlock()

if fileUpdatedExpectation != nil || treeRefreshedExpectation != nil {
    waitForExpectations(timeout: 5.0)
}

}
#endif

Expand Down