Skip to content

[Incremental, Testing] Implement 7 more removal tests. #702

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

Merged
merged 5 commits into from
Jun 7, 2021
Merged

[Incremental, Testing] Implement 7 more removal tests. #702

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8b1c124
prevent color codes in diagnostics
Jun 5, 2021
b96fb6d
Fix fencepost error in enumerating all cases
Jun 5, 2021
b5eac91
Test the other combos
Jun 5, 2021
ab1db13
add a FIXME
Jun 5, 2021
3e581ca
address review comments
Jun 7, 2021
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
184 changes: 130 additions & 54 deletions Tests/SwiftDriverTests/IncrementalCompilationTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ final class IncrementalCompilationTests: XCTestCase {
// "-v",
"-save-temps",
"-incremental",
"-no-color-diagnostics",
]
+ inputPathsAndContents.map {$0.0.pathString} .sorted()
}
Expand Down Expand Up @@ -336,8 +337,6 @@ extension IncrementalCompilationTests {
fileprivate enum RemovalTestOption: String, CaseIterable, Comparable, Hashable, CustomStringConvertible {
case
removeInputFromInvocation,
removeSourceFile,
removePreviouslyAddedInputFromOutputFileMap,
removeSwiftDepsFile,
restoreBadPriors

Expand All @@ -352,7 +351,7 @@ fileprivate enum RemovalTestOption: String, CaseIterable, Comparable, Hashable,
}
var mask: Int { 1 << intValue}
static let maxIntValue = allCases.map {$0.intValue} .max()!
static let maxCombinedValue = (1 << maxIntValue) - 1
static let maxCombinedValue = (1 << (maxIntValue + 1)) - 1

var description: String { rawValue }
}
Expand All @@ -379,7 +378,6 @@ extension IncrementalCompilationTests {

/// Someday, turn this test on and test all cases
func testRemovalInAllCases() throws {
throw XCTSkip("unimplemented")
try testRemoval(includeFailingCombos: true)
}

Expand All @@ -400,26 +398,16 @@ extension IncrementalCompilationTests {
}

private func testRemoval(_ options: RemovalTestOptions) throws {
guard !options.isEmpty else {return}
print("*** testRemoval \(options) ***", to: &stderrStream); stderrStream.flush()
setUp() // clear derived data, restore output file map
print("\n*** testRemoval \(options) ***", to: &stderrStream); stderrStream.flush()

let newInput = "another"
let topLevelName = "nameInAnother"
try testAddingInput(newInput: newInput, defining: topLevelName)
if options.contains(.removeSourceFile) {
removeInput(newInput)
}
if options.contains(.removeSwiftDepsFile) {
removeSwiftDeps(newInput)
}
if options.contains(.removePreviouslyAddedInputFromOutputFileMap) {
// FACTOR
OutputFileMapCreator.write(module: module,
inputPaths: inputPathsAndContents.map {$0.0},
derivedData: derivedDataPath,
to: OFM)
}
let includeInputInInvocation = !options.contains(.removeInputFromInvocation)

let removeInputFromInvocation = options.contains(.removeInputFromInvocation)
let removeSwiftDepsFile = options.contains(.removeSwiftDepsFile)
let restoreBadPriors = options.contains(.restoreBadPriors)
do {
let wrapperFn = options.contains(.restoreBadPriors)
? preservingPriorsDo
Expand All @@ -428,14 +416,16 @@ extension IncrementalCompilationTests {
try self.checkNonincrementalAfterRemoving(
removedInput: newInput,
defining: topLevelName,
includeInputInInvocation: includeInputInInvocation)
removeInputFromInvocation: removeInputFromInvocation,
removeSwiftDepsFile: removeSwiftDepsFile)
}
}
try checkRestorationOfIncrementalityAfterRemoval(
removedInput: newInput,
defining: topLevelName,
includeInputInInvocation: includeInputInInvocation,
afterRestoringBadPriors: options.contains(.restoreBadPriors))
removeInputFromInvocation: removeInputFromInvocation,
removeSwiftDepsFile: removeSwiftDepsFile,
afterRestoringBadPriors: restoreBadPriors)
}
}

Expand Down Expand Up @@ -700,25 +690,62 @@ extension IncrementalCompilationTests {
private func checkNonincrementalAfterRemoving(
removedInput: String,
defining topLevelName: String,
includeInputInInvocation: Bool
removeInputFromInvocation: Bool,
removeSwiftDepsFile: Bool
) throws {
let extraArguments = includeInputInInvocation
? [inputPath(basename: removedInput).pathString]
: []
try doABuild(
"after removal of \(removedInput)",
checkDiagnostics: true,
extraArguments: extraArguments,
expecting: [
let extraArguments = removeInputFromInvocation
? [] : [inputPath(basename: removedInput).pathString]

if removeSwiftDepsFile {
removeSwiftDeps(removedInput)
}
let expectations: [[Diagnostic.Message]]
switch (removeInputFromInvocation, removeSwiftDepsFile) {
case (false, false):
expectations = [
.readGraphAndSkipAll("main", "other", removedInput)
]
case
(true, false),
(true, true):
expectations = [
.remarks(
"Incremental compilation: Incremental compilation has been disabled, because the following inputs were used in the previous compilation but not in this one: \(removedInput).swift"),
.findingBatchingCompiling("main", "other"),
.linking,
],
]
case (false, true):
expectations = [
.readGraph,
.enablingCrossModule,
.maySkip("main", "other", removedInput),
.missing(removedInput),
.queuingInitial(removedInput),
.skipping("main", "other"),
.findingBatchingCompiling(removedInput),
.schedulingPostCompileJobs,
.linking,
.skipped("main", "other"),
]
}

let driver = try doABuild(
"after removal of \(removedInput)",
checkDiagnostics: true,
extraArguments: extraArguments,
expecting: expectations,
whenAutolinking: autolinkLifecycleExpectations)
.verifyNoGraph()

verifyNoPriors()
if removeInputFromInvocation {
driver.verifyNoGraph()
verifyNoPriors()
}
else {
let graph = try driver.moduleDependencyGraph()
graph.verifyGraph()
XCTAssert(graph.contains(sourceBasenameWithoutExt: removedInput))
XCTAssert(graph.contains(name: topLevelName))
}
}

/// Ensure that incremental builds happen after a removal.
Expand All @@ -730,34 +757,68 @@ extension IncrementalCompilationTests {
private func checkRestorationOfIncrementalityAfterRemoval(
removedInput: String,
defining topLevelName: String,
includeInputInInvocation: Bool,
removeInputFromInvocation: Bool,
removeSwiftDepsFile: Bool,
afterRestoringBadPriors: Bool
) throws -> ModuleDependencyGraph {
let extraArguments = includeInputInInvocation
? [inputPath(basename: removedInput).pathString]
: []
let expectations: [[Diagnostic.Message]] = afterRestoringBadPriors
? [
.readGraph,
.enablingCrossModule,
.skippingAll("main", "other"),
]
: [
.createdGraphFromSwiftdeps,
.enablingCrossModule,
.skippingAll("main", "other"),
]
let extraArguments = removeInputFromInvocation
? [] : [inputPath(basename: removedInput).pathString]
let inputs = ["main", "other"] + (removeInputFromInvocation ? [] : [removedInput])
let expectations: [[Diagnostic.Message]]
switch (removeInputFromInvocation, removeSwiftDepsFile, afterRestoringBadPriors) {
case
(false, false, false),
(false, true, false),
(false, false, true ),
(true, false, true ),
(true, true, true ),
(false, true, true ):
expectations = [
.readGraphAndSkipAll(inputs)
]
case
(true, false, false),
(true, true, false):
expectations = [
.createdGraphFromSwiftdeps,
.enablingCrossModule,
.skippingAll(inputs),
]
}

let graph = try doABuild(
"after after removal of \(removedInput)",
"restoring incrementality after removal of \(removedInput)",
checkDiagnostics: true,
extraArguments: extraArguments,
expecting: expectations,
whenAutolinking: autolinkLifecycleExpectations)
.moduleDependencyGraph()

graph.verifyGraph()
graph.ensureOmits(sourceBasenameWithoutExt: removedInput)
graph.ensureOmits(name: topLevelName)
if removeInputFromInvocation {
if afterRestoringBadPriors {
// FIXME: Fix the driver
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please add a little bit more detail to this FIXME?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great idea! Adding the following: ```
// If you incrementally compile with a.swift and b.swift,
// at the end, the driver saves a serialized ModuleDependencyGraph
// contains nodes for declarations defined in both files.
// If you then later remove b.swift and recompile, the driver will
// see that a file was removed (via comparisons with the saved `BuildRecord`
// and will delete the saved priors. However, if for some reason the
// saved priors are not deleted, the driver will read saved priors
// containing entries for the deleted file. This test simulates that
// condition by restoring the deleted priors. The driver ought to be fixed
// to cull any entries for removed files from the deserialized priors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me know if the above needs anything--thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good, thanks!

// If you incrementally compile with a.swift and b.swift,
// at the end, the driver saves a serialized `ModuleDependencyGraph`
// contains nodes for declarations defined in both files.
// If you then later remove b.swift and recompile, the driver will
// see that a file was removed (via comparisons with the saved `BuildRecord`
// and will delete the saved priors. However, if for some reason the
// saved priors are not deleted, the driver will read saved priors
// containing entries for the deleted file. This test simulates that
// condition by restoring the deleted priors. The driver ought to be fixed
// to cull any entries for removed files from the deserialized priors.
print("*** WARNING: skipping checks, driver fails to cleaned out the graph ***",
to: &stderrStream); stderrStream.flush()
return graph
}
graph.ensureOmits(sourceBasenameWithoutExt: removedInput)
graph.ensureOmits(name: topLevelName)
}
else {
XCTAssert(graph.contains(sourceBasenameWithoutExt: removedInput))
XCTAssert(graph.contains(name: topLevelName))
}

return graph
}
Expand Down Expand Up @@ -896,12 +957,14 @@ fileprivate extension ModuleDependencyGraph {
allNodes.contains {$0.contains(name: target)}
}
func ensureOmits(sourceBasenameWithoutExt target: String) {
// Written this way to show the faulty node when the assertion fails
nodeFinder.forEachNode { node in
XCTAssertFalse(node.contains(sourceBasenameWithoutExt: target),
"graph should omit source: \(target)")
}
}
func ensureOmits(name: String) {
// Written this way to show the faulty node when the assertion fails
nodeFinder.forEachNode { node in
XCTAssertFalse(node.contains(name: name),
"graph should omit decl named: \(name)")
Expand Down Expand Up @@ -1096,11 +1159,24 @@ fileprivate extension Array where Element == Diagnostic.Message {
static func skipped(_ inputs: String...) -> Self {
skipped(inputs)
}
static func skippingAll(_ inputs: String...) -> Self {
static func skippingAll(_ inputs: [String]) -> Self {
[
maySkip(inputs), skipping(inputs), skippingLinking, skipped(inputs)
].flatMap {$0}
}
static func skippingAll(_ inputs: String...) -> Self {
skippingAll(inputs)
}
static func readGraphAndSkipAll(_ inputs: [String]) -> Self {
[
readGraph,
enablingCrossModule,
skippingAll(inputs)
].flatMap{$0}
}
static func readGraphAndSkipAll(_ inputs: String...) -> Self {
readGraphAndSkipAll(inputs)
}

// MARK: - batching
static func addingToBatch(_ inputs: [String], _ b: Int) -> Self {
Expand Down