Speed up directory monitoring tests

[snprajwal]

Summary

The directory monitoring tests check for whether the documentation is rebuilt if the documentation catalog is edited when running a live server with docc preview. These tests assumed an extremely generous timeout of 5 seconds for changes to the directory, and 10 seconds for a lack of change. Thus, they ran much slower than the rest of the test suite. The timeouts have been reduced to a more reasonable threshold of 1.5 seconds, which still succeeds with throttled CPU resources.

Dependencies

N/A

Testing

This was tested by restricting CPU resources to 20% of a single core and running the tests 100 times:

for i in $(seq 1 100); do cpulimit -i -l 20 swift test --filter DirectoryMonitor; done

Checklist

Make sure you check off the following items. If they cannot be completed, provide a reason.

• [ ] Added tests
• Ran the ./bin/test script and it succeeded
• [ ] Updated documentation if necessary

[snprajwal]

@swift-ci please test

[d-ronnqvist]

Only one of these changes make the directory monitor test faster, the rest only make them more fragile without running faster.

[d-ronnqvist]

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.

[snprajwal]

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.

[d-ronnqvist]

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.

[d-ronnqvist]

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)

[d-ronnqvist]

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)
}

[snprajwal]

@swift-ci please test