Fix occasional flakiness in preview action cancellation test #1070
Conversation
d-ronnqvist
force-pushed
the
fix-flaky-preview-cancellation-test
branch
from
13359b4 to
cc7ad01
Compare
rdar://137751315
d-ronnqvist
force-pushed
the
fix-flaky-preview-cancellation-test
branch
from
cc7ad01 to
756cb7e
Compare
| @@ -51,7 +54,7 @@ public enum LogHandle: TextOutputStream { | |||
| case .file(let fileHandle): | |||
| fileHandle.write(Data(string.utf8)) | |||
| case .memory(let storage): | |||
| storage.text.append(string) | |||
| storage._text.sync { $0.append(string) } | |||
There was a problem hiding this comment.
Do we need to synchronize fileHandle in the same way? Or does macOS/Linux guarantee that writing to a file is thread safe - i.e. synchronized at a lower level?
Same question for stdout and stderr...
There was a problem hiding this comment.
I haven't ever encountered any issues with writing to a file and I've never seen a pattern of synchronizing around fileHandle.write or print but I can't say for sure.
|
|
||
| public extension Lock { |
There was a problem hiding this comment.
Is it safe to mark this extension as non-public? Could some client package be using it?
There was a problem hiding this comment.
Yes, it's safe because it's extending an internal type so its API was never actually available to the public.
Because Lock was defined as typealias Lock = Synchronized<Void> (no access modifier) it was never publicly available so a public extension to Lock could never exceed the Lock types internal access. I feel that there should have been a warning about this but there wasn't.
There was a problem hiding this comment.
I only found this because I tried to use this API from SwiftDocCUtilities and got a build error that it could't be accessed because it was internal.
| self.monitoredConvertTask?.cancel() | ||
| self.monitoredConvertTask = Task { | ||
| defer { |
There was a problem hiding this comment.
Why don't we need this defer block any more?
There was a problem hiding this comment.
From what I can tell it wasn't ever needed. The DirectoryMonitor already does a smarter automatic reloading when files are added to or removed from a monitored directory.
Removing the restart here means that we don't:
- Restart (stop monitoring all files and directories and start monitoring them again) twice when a file is added removed (once here and once in the directory monitor itself).
- Restart (stop monitoring all files and directories and start monitoring them again) when an already monitored file's content changed but no files were added or removed.
| // If we couldn't create short enough socket path, skip the tests rather than fail them to avoid flakiness in the CI. | ||
| // The current implementation _should_ result in a path that's around 92 characters long, so the 11 character headroom | ||
| // should cover some amount of hypothetical changes to the `NSTemporaryDirectory` length and the `UUID().uuidString` length. | ||
| try XCTSkipIf(!isShortEnoughUnixDomainSocket(socketURL), |
There was a problem hiding this comment.
This is an interesting XCTest feature: Skipping a test after it has started!
There was a problem hiding this comment.
Yes. It's typically done as the first step of a test but here I'm using it to stop before a detectable "failure" that's more caused by the environment in which the tests are executed than the test itself.
|
@swift-ci please test |
Bug/issue #, if applicable: rdar://137751315
Summary
This fixes an occasional flakiness issue in
PreviewActionIntegrationTests/testCancelsConversion.AFAICT there were a few different issues that this test could encounter:
LogHandle/LogStoragecaused the test to trap instead of accumulating log outputDirectoryMonitorcaused the test to trap.logHandlevariable caused the test to trap. This was fixed by not synchronizing access within the preview action and passing separate copies to thePreviewServerandConvertAction.loadErrorvariable inSymbolGraphLoaderNIOCore/SocketAddressError/unixDomainSocketPathTooLongerror. This was fixed by creating a shorter temporary unix domain socket path for these tests and opting to skip the test if it can't create a short enough path.After all those changes I revisited the removed
testWatchRecoversAfterConversionErrorstest and reimplemented it in the style oftestCancelsConversion. AFACIT the in this PR addresses the flakiness in both tests.Dependencies
None
Testing
for i in {1..100}; do swift test --parallel --filter SwiftDocCUtilitiesTests.PreviewActionIntegrationTests; doneI let this run over 5900 times (over night) without encountering any failures so I'm reasonably sure that most of the flakiness is gone 🤞
Checklist
Make sure you check off the following items. If they cannot be completed, provide a reason.
AddedUpdated tests./bin/testscript and it succeeded[ ] Updated documentation if necessaryNot applicable