Skip to content

Fix test failures after merging rebranch #923

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 1 commit into from
Oct 26, 2023
Merged

Fix test failures after merging rebranch #923

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
41 changes: 24 additions & 17 deletions Tests/SourceKitLSPTests/BuildSystemTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,8 +123,6 @@ final class BuildSystemTests: XCTestCase {
// MARK: - Tests

func testClangdDocumentUpdatedBuildSettings() async throws {
try XCTSkipIf(true, "rdar://115435598 - crashing on rebranch")

guard haveClangd else { return }

let doc = DocumentURI.for(.objective_c)
Expand Down Expand Up @@ -155,14 +153,17 @@ final class BuildSystemTests: XCTestCase {
let newSettings = FileBuildSettings(compilerArguments: args + ["-DFOO"])
buildSystem.buildSettingsByFile[doc] = newSettings

let expectation = XCTestExpectation(description: "refresh")
let refreshedDiags = try await testClient.nextDiagnosticsNotification()
XCTAssertEqual(refreshedDiags.diagnostics.count, 0)
XCTAssertEqual(text, documentManager.latestSnapshot(doc)!.text)

await buildSystem.delegate?.fileBuildSettingsChanged([doc])

try await fulfillmentOfOrThrow([expectation])
var receivedCorrectDiagnostic = false
for _ in 0..<Int(defaultTimeout) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems kind of weird to me to base this on timeout

Copy link
Member Author

Choose a reason for hiding this comment

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

It makes sense to me. We wait 1 second for each notification and iterate this defaultTimeout times. That means that we wait for the correct diagnostic notification defaultTimeout seconds.

Copy link
Contributor

Choose a reason for hiding this comment

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

Which only works if the number of notifications received is < defaultTimeout in seconds. Right now there's 2 so that's fine, but that's why it feels odd to me - the number of notifications we allow is dependent on the timeout value, rather than just receiving notifications up to a timeout/successful notification.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, the assumption is that number of notifications received ≪ default timeout. I think that’s a fair assumption to make. Happy to change it to 0..<30 though, like we do in other test case.

let refreshedDiags = try await testClient.nextDiagnosticsNotification(timeout: 1)
if refreshedDiags.diagnostics.count == 0, text == documentManager.latestSnapshot(doc)!.text {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the document snapshot even tested in these tests 🤔?

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably to check that we get no diagnostics for the expected source file contents and not, e.g. no diagnostics because the file is empty. I probably wouldn’t have checked it if I wrote the test but 🤷🏽

Copy link
Contributor

Choose a reason for hiding this comment

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

But that's not even what that is testing, unless I'm misunderstanding it. All it's checking is that our state in document manager matches what we expect, which isn't really anything to do with diagnostics.

Copy link
Member Author

Choose a reason for hiding this comment

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

Happy to remove the check if you care.

Copy link
Contributor

Choose a reason for hiding this comment

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

Unless you can actually see a reason for it, IMO it just confuses what the test is testing. So yeah, I'd be in favor of removing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Happy to leave it, but can you throw in a comment saying why we're doing this? Preferably with a link to the radar about fixing this in clangd, since it really does seem like a bug.

Copy link
Member Author

Choose a reason for hiding this comment

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

I haven’t been able to find a good clangd reproducer yet outside of this test case, so this might actually be a race within the test itself. Since I want to re-write these tests with the new test workspace infrastructure anyway, I’ll leave it as-is for now and will add a comment to a clangd bug if the issue still persists with the new test infrastructure.

receivedCorrectDiagnostic = true
break
}
}
XCTAssert(receivedCorrectDiagnostic)
}

func testSwiftDocumentUpdatedBuildSettings() async throws {
Expand Down Expand Up @@ -269,18 +270,16 @@ final class BuildSystemTests: XCTestCase {
}

func testMainFilesChanged() async throws {
try XCTSkipIf(true, "rdar://115176405 - failing on rebranch due to extra published diagnostic")

let ws = try await mutableSourceKitTibsTestWorkspace(name: "MainFiles")!
let unique_h = ws.testLoc("unique").docIdentifier.uri

try ws.openDocument(unique_h.fileURL!, language: .cpp)

let openDiags = try await testClient.nextDiagnosticsNotification()
let openDiags = try await ws.testClient.nextDiagnosticsNotification()
XCTAssertEqual(openDiags.diagnostics.count, 0)

try ws.buildAndIndex()
let diagsFromD = try await testClient.nextDiagnosticsNotification()
let diagsFromD = try await ws.testClient.nextDiagnosticsNotification()
XCTAssertEqual(diagsFromD.diagnostics.count, 1)
let diagFromD = try XCTUnwrap(diagsFromD.diagnostics.first)
XCTAssertEqual(diagFromD.severity, .warning)
Expand All @@ -301,11 +300,19 @@ final class BuildSystemTests: XCTestCase {
)
}

let diagsFromC = try await testClient.nextDiagnosticsNotification()
XCTAssertEqual(diagsFromC.diagnostics.count, 1)
let diagFromC = try XCTUnwrap(diagsFromC.diagnostics.first)
XCTAssertEqual(diagFromC.severity, .warning)
XCTAssertEqual(diagFromC.message, "UNIQUE_INCLUDED_FROM_C")
var receivedCorrectDiagnostic = false
for _ in 0..<Int(defaultTimeout) {
let diagsFromC = try await ws.testClient.nextDiagnosticsNotification(timeout: 1)
guard diagsFromC.diagnostics.count == 1, let diagFromC = diagsFromC.diagnostics.first else {
continue
}
guard diagFromC.severity == .warning, diagFromC.message == "UNIQUE_INCLUDED_FROM_C" else {
continue
}
receivedCorrectDiagnostic = true
break
}
XCTAssert(receivedCorrectDiagnostic)
}

private func clangBuildSettings(for uri: DocumentURI) -> FileBuildSettings {
Expand Down