[GC] Update a GC end-to-end test to use sinon timers instead of real ones

[agarwal-navin]

This PR updates one of the GC end-to-end test to use sinon fake timers instead of real timers. The tests in this file adds delays to wait for sweep timers to expire. Using fake timers these delays have been replaced with advancing the clock thereby reducing the runtime.
The run time for this test file went from 4s to 2s.

AB#11248

[Copilot]

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

packages/test/test-end-to-end-tests/src/test/gc/gcSweepDataStores.spec.ts:685

• Replace 'tombstone ready' with 'sweep ready' for consistency.

// Summarize. The tombstone ready data store should get realized because it has a trailing op.

[agarwal-navin]

The workaround to avoid this was to close the container that created the data store being deleted. However, on removing the delays, the test failied and I realized that closing the container causes the trailing op it sent to be not picked up by the LoaderContainerTracker. So, when the next summary happens, ensureSynchronized does not wait for the trailing op.

This should not have worked before this change either but for some reason it does maybe due to the awaits. My theory is that the op is likely processed by the summarizer by the time ensureSynchronized is called.

[agarwal-navin]

So, in this change I removed closing the container and added this itExpects. I think we can remove logging the GC_Deleted_DataStore_Unexpected_Delete error.

[markfields]

Could you have added a flush to the outbound queue or something? Like provider.processOutgoing? That waits for all ops to be sequenced and received (but not processed) by all tracked containers.

Also, I don't think we should remove that log. It's an important assumption that we should protect, so it doesn't accidentally regress with any of the architectural changes that may come to the runtime in the next year+.

[agarwal-navin]

While incorporating #23516 (comment), I used a workaround to not do this.

[markfields]

nice!

[markfields]

I wouldn't assume this does anything, but if the tests pass maybe it does? Did you read about it or experiment at all?

[markfields]

I think you want tickAsync?

    /**
     * Advance the clock, firing callbacks if necessary.
     *
     * @param time   How many ticks to advance by.
     * @returns Fake milliseconds since the unix epoch.
     */
    tick: (time: number | string) => number;

    /**
     * Advance the clock, firing callbacks if necessary.
     *
     * Also breaks the event loop, allowing any scheduled promise callbacks to execute _before_ running the timers.
     *
     * @param time   How many ticks to advance by.
     * @returns Fake milliseconds since the unix epoch.
     */
    tickAsync: (time: number | string) => Promise<number>;

[agarwal-navin]

I did use tickAsync. Did you mean to say tick?

[agarwal-navin]

I did not experiment a lot, no. This worked so I just used it.

[markfields]

We should be able to stop overriding the timeouts now, and just use the default values. I think that would be preferable if it works (and good to know why if it doesn't)

[agarwal-navin]

Good point. I changed it to use the default values. I had to re-arrange some logic but it worked. Session expiry is what makes it harder but I worked around it.