-
Notifications
You must be signed in to change notification settings - Fork 644
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.
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
Fix broken tests #2621
Fix broken tests #2621
Conversation
counter += 1 | ||
|
||
if next < maxSequenceValue { | ||
// This loop will exit when the task is cancelled | ||
// or when all of the elements have been iterated. | ||
// It is possible that the cancellation will be | ||
// triggered after the last element was yielded, | ||
// just before the iterator can return the `nil` | ||
// signifying the end of the sequence. | ||
// If this happens, we avoid incrementing the counter, | ||
// so that we don't increase it to a value beyond the | ||
// maximum possible sequence value. | ||
counter += 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't quite follow that change nor the comment. The counter increment is happening inside the for await
loop. The AsyncSequence
that we are iterating over has exactly 100 elements and regardless when we are cancelled we should never count to above the element size of the Array
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're not counting above the size of the array - the problem is with how we're indexing. By the time the sequence yields 99, counter
will also have a value of 99. However, at the end of the iteration, we increment the counter to 100, bringing us past the limit and failing the assertion XCTAssertLessThan(counter, 100)
.
The confusion comes from the fact that counter
is counting elements yielded/iterations completed, but we assert its value matches the yielded value from the sequence, which starts at 0.
There are several ways of fixing this, and I agree this was probably not very clear. I've changed it to something that is hopefully more obvious now.
Motivation:
This PR fixes several broken/flaky unit and integration tests, that started failing in our nightly pipeline.
Namely:
NIOThrowingAsyncSequenceTests.testIteratorThrows_whenCancelled
: unit test was flaky.EventLoopCrashTests.testInstallingSingletonMTELGAsConcurrencyExecutorWorksButOnlyOnce
: integration test failed in nightly.Modifications:
EventLoopCrashTests.testInstallingSingletonMTELGAsConcurrencyExecutorWorksButOnlyOnce
was failing because we forgot to disable the test for Swift versions greater than 5.10. There isn't sufficient testing done on this feature on versions greater than 5.10, so the whole feature is simply disabled in those cases, so the test should be too.AtomicCounter
was not an explicit dependency of the test targets. This worked before because all modules were always visible to targets, but this changed in Move .swiftmodule output directory swiftlang/swift-package-manager#7103. I addedAtomicCounter
as a dependency to these targets and the build now succeeds.swift-tools-version
in generatedPackage.swift
s to 5.7. This wasn't necessary to fixing the issues but they were lagging behind by a lot (some were version 5.0).Result:
Happy nightly pipeline.