Fix test failures after merging rebranch #923
Conversation
AFAICT, when chainging build settings `clangd` now provides two `PublishDiagnosticNotifications`: One with the diagnostics that were still computed from the old build settings, followed by a notification that contains the diagnostics for the new build settings. Relax the tests so that we don’t expect the *next* diagnsotics notification to contain the the updated diagnostics but just expect to receive any diagnostic notification within a timeout that contains the updated diagnostics. rdar://115435598
|
@swift-ci Please test |
| await buildSystem.delegate?.fileBuildSettingsChanged([doc]) | ||
|
|
||
| try await fulfillmentOfOrThrow([expectation]) | ||
| var receivedCorrectDiagnostic = false | ||
| for _ in 0..<Int(defaultTimeout) { |
There was a problem hiding this comment.
Seems kind of weird to me to base this on timeout
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| var receivedCorrectDiagnostic = false | ||
| for _ in 0..<Int(defaultTimeout) { | ||
| let refreshedDiags = try await testClient.nextDiagnosticsNotification(timeout: 1) | ||
| if refreshedDiags.diagnostics.count == 0, text == documentManager.latestSnapshot(doc)!.text { |
There was a problem hiding this comment.
Why is the document snapshot even tested in these tests 🤔?
There was a problem hiding this comment.
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 🤷🏽
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Happy to remove the check if you care.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
AFAICT, when chainging build settings
clangdnow provides twoPublishDiagnosticNotifications: One with the diagnostics that were still computed from the old build settings, followed by a notification that contains the diagnostics for the new build settings.Relax the tests so that we don’t expect the next diagnsotics notification to contain the the updated diagnostics but just expect to receive any diagnostic notification within a timeout that contains the updated diagnostics.
rdar://115435598