Skip to content

Commit 22c4a11

Browse files
authored
fix flaky tests that assume order on DispatchQueue.async (#3806)
motivation: stable tests changes: * update ManifestLoader to call willParse delegate call on the same queue as wilLoad, for correctnessa dn consistency * update manifest loading tests to wait on delegate calls since they may come out of order: ManifestLoaded uses the same queue to call the delegates as it does for calling the delegate. rdar://79415639 rdar://84181195
1 parent 2af5115 commit 22c4a11

File tree

2 files changed

+75
-46
lines changed

2 files changed

+75
-46
lines changed

Sources/PackageLoading/ManifestLoader.swift

Lines changed: 17 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -247,6 +247,7 @@ public final class ManifestLoader: ManifestLoaderProtocol {
247247
packageKind: packageKind,
248248
toolsVersion: toolsVersion,
249249
identityResolver: identityResolver,
250+
delegateQueue: queue,
250251
fileSystem: fileSystem,
251252
diagnostics: diagnostics)
252253

@@ -498,6 +499,7 @@ public final class ManifestLoader: ManifestLoaderProtocol {
498499
packageKind: PackageReference.Kind,
499500
toolsVersion: ToolsVersion,
500501
identityResolver: IdentityResolver,
502+
delegateQueue: DispatchQueue,
501503
fileSystem: FileSystem,
502504
diagnostics: DiagnosticsEngine?
503505
) throws -> ManifestJSONParser.Result {
@@ -543,10 +545,13 @@ public final class ManifestLoader: ManifestLoaderProtocol {
543545
}
544546

545547
// shells out and compiles the manifest, finally output a JSON
546-
let result = self.evaluateManifest(packageIdentity: key.packageIdentity,
547-
manifestPath: key.manifestPath,
548-
manifestContents: key.manifestContents,
549-
toolsVersion: key.toolsVersion)
548+
let result = self.evaluateManifest(
549+
packageIdentity: key.packageIdentity,
550+
manifestPath: key.manifestPath,
551+
manifestContents: key.manifestContents,
552+
toolsVersion: key.toolsVersion,
553+
delegateQueue: delegateQueue
554+
)
550555

551556
// only cache successfully parsed manifests
552557
let parseManifest = try self.parseManifest(
@@ -653,7 +658,8 @@ public final class ManifestLoader: ManifestLoaderProtocol {
653658
packageIdentity: PackageIdentity,
654659
manifestPath: AbsolutePath,
655660
manifestContents: [UInt8],
656-
toolsVersion: ToolsVersion
661+
toolsVersion: ToolsVersion,
662+
delegateQueue: DispatchQueue
657663
) -> EvaluationResult {
658664

659665
var result = EvaluationResult()
@@ -663,6 +669,7 @@ public final class ManifestLoader: ManifestLoaderProtocol {
663669
at: manifestPath,
664670
packageIdentity: packageIdentity,
665671
toolsVersion: toolsVersion,
672+
delegateQueue: delegateQueue,
666673
result: &result
667674
)
668675
} else {
@@ -672,6 +679,7 @@ public final class ManifestLoader: ManifestLoaderProtocol {
672679
at: tempFile.path,
673680
packageIdentity: packageIdentity,
674681
toolsVersion: toolsVersion,
682+
delegateQueue: delegateQueue,
675683
result: &result
676684
)
677685
}
@@ -689,9 +697,12 @@ public final class ManifestLoader: ManifestLoaderProtocol {
689697
at manifestPath: AbsolutePath,
690698
packageIdentity: PackageIdentity,
691699
toolsVersion: ToolsVersion,
700+
delegateQueue: DispatchQueue,
692701
result: inout EvaluationResult
693702
) throws {
694-
self.delegate?.willParse(manifest: manifestPath)
703+
delegateQueue.async {
704+
self.delegate?.willParse(manifest: manifestPath)
705+
}
695706

696707
// The compiler has special meaning for files with extensions like .ll, .bc etc.
697708
// Assert that we only try to load files with extension .swift to avoid unexpected loading behavior.

Tests/PackageLoadingTests/PD_4_2_LoadingTests.swift

Lines changed: 58 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
*/
1010

1111
import Basics
12+
import Dispatch
1213
import PackageLoading
1314
import PackageModel
1415
import SPMTestSupport
@@ -542,11 +543,6 @@ class PackageDescription4_2LoadingTests: PackageDescriptionLoadingTests {
542543
}
543544

544545
func testCacheInvalidationOnEnv() throws {
545-
#if os(Linux)
546-
// rdar://79415639 (Test Case 'PackageDescription4_2LoadingTests.testCacheInvalidationOnEnv' failed)
547-
try XCTSkipIf(true)
548-
#endif
549-
550546
try testWithTemporaryDirectory { path in
551547
let fs = localFileSystem
552548

@@ -571,15 +567,17 @@ class PackageDescription4_2LoadingTests: PackageDescriptionLoadingTests {
571567

572568
func check(loader: ManifestLoader, expectCached: Bool) {
573569
delegate.clear()
570+
delegate.prepare(expectParsing: !expectCached)
571+
574572
let manifest = try! loader.load(
575573
at: manifestPath.parentDirectory,
576574
packageKind: .fileSystem(manifestPath.parentDirectory),
577575
toolsVersion: .v4_2,
578576
fileSystem: fs
579577
)
580578

581-
XCTAssertEqual(delegate.loaded, [manifestPath])
582-
XCTAssertEqual(delegate.parsed, expectCached ? [] : [manifestPath])
579+
XCTAssertEqual(try delegate.loaded(timeout: .now() + 1), [manifestPath])
580+
XCTAssertEqual(try delegate.parsed(timeout: .now() + 1), expectCached ? [] : [manifestPath])
583581
XCTAssertEqual(manifest.name, "Trivial")
584582
XCTAssertEqual(manifest.targets[0].name, "foo")
585583
}
@@ -626,15 +624,17 @@ class PackageDescription4_2LoadingTests: PackageDescriptionLoadingTests {
626624

627625
func check(loader: ManifestLoader, expectCached: Bool) {
628626
delegate.clear()
627+
delegate.prepare(expectParsing: !expectCached)
628+
629629
let manifest = try! loader.load(
630630
at: manifestPath.parentDirectory,
631631
packageKind: .fileSystem(manifestPath.parentDirectory),
632632
toolsVersion: .v4_2,
633633
fileSystem: fs
634634
)
635635

636-
XCTAssertEqual(delegate.loaded, [manifestPath])
637-
XCTAssertEqual(delegate.parsed, expectCached ? [] : [manifestPath])
636+
XCTAssertEqual(try delegate.loaded(timeout: .now() + 1), [manifestPath])
637+
XCTAssertEqual(try delegate.parsed(timeout: .now() + 1), expectCached ? [] : [manifestPath])
638638
XCTAssertEqual(manifest.name, "Trivial")
639639
XCTAssertEqual(manifest.targets[0].name, "foo")
640640
}
@@ -709,18 +709,27 @@ class PackageDescription4_2LoadingTests: PackageDescriptionLoadingTests {
709709
XCTAssertEqual(m.name, "Trivial")
710710
}
711711

712-
try check(loader: manifestLoader)
713-
XCTAssertEqual(delegate.loaded.count, 1)
714-
XCTAssertEqual(delegate.parsed.count, 1)
712+
do {
713+
delegate.prepare()
714+
try check(loader: manifestLoader)
715+
XCTAssertEqual(try delegate.loaded(timeout: .now() + 1).count, 1)
716+
XCTAssertEqual(try delegate.parsed(timeout: .now() + 1).count, 1)
717+
}
715718

716-
try check(loader: manifestLoader)
717-
XCTAssertEqual(delegate.loaded.count, 2)
718-
XCTAssertEqual(delegate.parsed.count, 1)
719+
do {
720+
delegate.prepare(expectParsing: false)
721+
try check(loader: manifestLoader)
722+
XCTAssertEqual(try delegate.loaded(timeout: .now() + 1).count, 2)
723+
XCTAssertEqual(try delegate.parsed(timeout: .now() + 1).count, 1)
724+
}
719725

720-
stream <<< "\n\n"
721-
try check(loader: manifestLoader)
722-
XCTAssertEqual(delegate.loaded.count, 3)
723-
XCTAssertEqual(delegate.parsed.count, 2)
726+
do {
727+
stream <<< "\n\n"
728+
delegate.prepare()
729+
try check(loader: manifestLoader)
730+
XCTAssertEqual(try delegate.loaded(timeout: .now() + 1).count, 3)
731+
XCTAssertEqual(try delegate.parsed(timeout: .now() + 1).count, 2)
732+
}
724733
}
725734
}
726735

@@ -800,6 +809,7 @@ class PackageDescription4_2LoadingTests: PackageDescriptionLoadingTests {
800809
let identityResolver = DefaultIdentityResolver()
801810

802811
// warm up caches
812+
delegate.prepare()
803813
let manifest = try tsc_await { manifestLoader.load(at: manifestPath.parentDirectory,
804814
packageIdentity: .plain("Trivial"),
805815
packageKind: .fileSystem(manifestPath.parentDirectory),
@@ -818,6 +828,7 @@ class PackageDescription4_2LoadingTests: PackageDescriptionLoadingTests {
818828
let sync = DispatchGroup()
819829
for _ in 0 ..< total {
820830
sync.enter()
831+
delegate.prepare(expectParsing: false)
821832
manifestLoader.load(at: manifestPath.parentDirectory,
822833
packageIdentity: .plain("Trivial"),
823834
packageKind: .fileSystem(manifestPath.parentDirectory),
@@ -845,7 +856,7 @@ class PackageDescription4_2LoadingTests: PackageDescriptionLoadingTests {
845856
XCTFail("timeout")
846857
}
847858

848-
XCTAssertEqual(delegate.loaded.count, total+1)
859+
XCTAssertEqual(try delegate.loaded(timeout: .now() + 1).count, total+1)
849860
XCTAssertFalse(observability.hasWarningDiagnostics, observability.diagnostics.description)
850861
XCTAssertFalse(observability.hasErrorDiagnostics, observability.diagnostics.description)
851862
}
@@ -882,6 +893,7 @@ class PackageDescription4_2LoadingTests: PackageDescriptionLoadingTests {
882893
}
883894

884895
sync.enter()
896+
delegate.prepare()
885897
manifestLoader.load(at: manifestPath.parentDirectory,
886898
packageIdentity: .plain("Trivial-\(random)"),
887899
packageKind: .fileSystem(manifestPath.parentDirectory),
@@ -909,46 +921,52 @@ class PackageDescription4_2LoadingTests: PackageDescriptionLoadingTests {
909921
XCTFail("timeout")
910922
}
911923

912-
XCTAssertEqual(delegate.loaded.count, total)
924+
XCTAssertEqual(try delegate.loaded(timeout: .now() + 1).count, total)
913925
XCTAssertFalse(observability.hasWarningDiagnostics, observability.diagnostics.description)
914926
XCTAssertFalse(observability.hasErrorDiagnostics, observability.diagnostics.description)
915927
}
916928
}
917929

918930
final class ManifestTestDelegate: ManifestLoaderDelegate {
919-
private let lock = Lock()
920-
private var _loaded: [AbsolutePath] = []
921-
private var _parsed: [AbsolutePath] = []
931+
private let loaded = ThreadSafeArrayStore<AbsolutePath>()
932+
private let parsed = ThreadSafeArrayStore<AbsolutePath>()
933+
private let loadingGroup = DispatchGroup()
934+
private let parsingGroup = DispatchGroup()
922935

923-
func willLoad(manifest: AbsolutePath) {
924-
self.lock.withLock {
925-
self._loaded.append(manifest)
936+
func prepare(expectParsing: Bool = true) {
937+
self.loadingGroup.enter()
938+
if expectParsing {
939+
self.parsingGroup.enter()
926940
}
927941
}
928942

943+
func willLoad(manifest: AbsolutePath) {
944+
self.loaded.append(manifest)
945+
self.loadingGroup.leave()
946+
}
947+
929948
func willParse(manifest: AbsolutePath) {
930-
self.lock.withLock {
931-
self._parsed.append(manifest)
932-
}
949+
self.parsed.append(manifest)
950+
self.parsingGroup.leave()
933951
}
934952

935953
func clear() {
936-
self.lock.withLock {
937-
self._loaded.removeAll()
938-
self._parsed.removeAll()
939-
}
954+
self.loaded.clear()
955+
self.parsed.clear()
940956
}
941957

942-
var loaded: [AbsolutePath] {
943-
self.lock.withLock {
944-
self._loaded
958+
func loaded(timeout: DispatchTime) throws -> [AbsolutePath] {
959+
guard case .success = self.loadingGroup.wait(timeout: timeout) else {
960+
throw StringError("timeout waiting for loading")
945961
}
962+
return self.loaded.get()
946963
}
947964

948-
var parsed: [AbsolutePath] {
949-
self.lock.withLock {
950-
self._parsed
965+
func parsed(timeout: DispatchTime) throws -> [AbsolutePath] {
966+
guard case .success = self.parsingGroup.wait(timeout: timeout) else {
967+
throw StringError("timeout waiting for parsing")
951968
}
969+
return self.parsed.get()
952970
}
953971
}
954972
}

0 commit comments

Comments
 (0)