Repository navigation
Mock clock - #191
Mock clock#191
Conversation
42c686f to
91066e0
Compare
| swiftSettings: availabilityMacros + settings | ||
| ), | ||
| .target( | ||
| name: "SwiftNetworkTestSupport", |
There was a problem hiding this comment.
Could we put the support functionality into SwiftNetworkTestHarness? Happy to rename that module, but it seems pretty much the same overall purpose
There was a problem hiding this comment.
I think they have a different purpose, and I had actually omitted something initially which made that unclear. I think that the new target should be a product, so that external adopters can also use mock time in their tests if needed.
agnosticdev
left a comment
agnosticdev
left a comment
There was a problem hiding this comment.
In which cases will we adopt this? Will it be strictly in our unit tests or will it be in QUIC stack tests as well? The reason I ask is that for the tests that are exercising loss recovery scenarios it would be good to still use real time for those cases.
e7c10cc to
03cb069
Compare
So this PR only converts the unit tests that build a context and never wait on anything. My intention is to move the QUIC stack tests over in follow-ups, loss recovery included. The intention is that everyone agrees on one source of truth for time, so that to any code reading the clock through the context, advancing it by hand is indistinguishable from the wall clock advancing. I'd like all our tests on mocked time eventually, with two exceptions: anything driving real sockets, and the handful that exist to exercise the real dispatch timer — On loss recovery specifically: nothing in those tests is real already. The transport is an in-memory bridge, loss comes from Do you see any issues with this plan which I'm missing? |
My concern is really the Recovery and Ack timers firing to run actions such as delayed ack, PTO, etc.. |
I agree with this approach. It would be nice though if we could have an option to do an integration test with the real clock. We wouldn't use it by default, but we could use it when we are tracking bugs and want to run a test with real time. |
To keep coverage of real time we could run with real clocks overnight for some or all tests so that the longer duration isn't an issue. The risk there is that this comes with all of the potential flakiness issues mocked time hopes to resolve, so triaging them could be painful. |
Agree with the flakiness concern. I would recommend that we defer the nightly real clock run to later if we really think we must. |
I think we need some sort of real time tests here still. This has led us to find race conditions in the past. Why would triaging them be painful here? |
|
Just because racy tests are IMO painful in general. Tests which fail sometimes on some machines, and maybe not if you put in a debug line here or run in a debugger; generally not a fan of those. It's not always clear if it's the test or the code which has some corner-case issue and it's hard to investigate. |
agnosticdev
left a comment
agnosticdev
left a comment
There was a problem hiding this comment.
As long as we can create a nightly run with a sub-set of loss recovery tests that do exercise real time I am fine with this.
A call site wanting parameters on a particular context had to declare them `var` and reassign `context` on the next line, whether or not it went on to change anything else. `Parameters.context` is already public, so this adds no surface beyond the initializer itself. * Added `Parameters.init(context:)` outside the platform conditionals, so the private and embedded builds have it as well as the open-source one. * Adopted it at 15 call sites, dropping the `context` reassignment from each. * Switched ten of those sites to `let`, since they no longer mutate the value.
`ManualScheduler` and `ManualTimeContext` let a test drive the stack's timers without reading a real clock, now that the scheduler owns time. They sit outside the library so nothing in it depends on them, and are offered as their own product so an adopter can drive time in tests of their own code. * Added `run(until:)` analogous to XCTest's `wait(for:timeout:)`, reporting whether the condition held rather than blocking on real time. * Mocking fires each timer with `now` set to its own deadline, so a timer that reads the clock sees the instant it was scheduled for rather than the end of the advance. * Moved `NetworkClock.Instant.testBase` into the new product and deleted `Tests/QUICTests/TestClock.swift`, which held nothing else. * Imported the new product in the five suites that use `testBase`. * Added `ManualSchedulerTests` to cover the scheduler.
`testBase` sat beside `ManualTimeContext` but needs nothing from it: it is a fixed instant built from `NetworkClock.Instant` alone. Sharing a file means a build that can compile the constant but cannot supply a scheduler ends up with neither. * Moved `testBase` into `TestInstants.swift`.
e067315 to
bafc504
Compare
4 participants
Add a test-support target for driving virtual time
ManualSchedulerandManualTimeContextlet a test drive the stack's timers without reading a real clock, now that the scheduler owns time. They need a home outside the library, so nothing ships them, and outside either test target, so both can import them.run(until:)analogous to XCTest'swait(for:timeout:), reporting whether the condition held rather than blocking on real time.nowset to its own deadline, so a timer that reads the clock sees the instant it was scheduled for rather than the end of the advance.NetworkClock.Instant.testBaseinto the new target and deletedTests/QUICTests/TestClock.swift, which held nothing else.testBase.ManualSchedulerTeststo cover the scheduler.NOTE: this is not yet adopted, that comes in a follow-up PR