Skip to content

(#384) added pull progress report via event OfflineDbContext.SynchronizationProgress #387

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

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

StefanCuypers
Copy link
Contributor

OfflineDbContext.SynchronizationProgress event is added to provide progress report during pull operations. Both source fetching and local committing progress are reported.

Copilot

This comment was marked as outdated.

@adrianhall
Copy link
Collaborator

This looks good - I'll take the compile checks on later this week.

Do you want to add a PullOperationStart and PullOperationEnd event as well? I'm ambivalent to the PullOperationStart, but I think it would be useful for the async process to understand when they won't be getting more events.

@adrianhall adrianhall linked an issue Jun 30, 2025 that may be closed by this pull request
Copy link
Collaborator

@adrianhall adrianhall left a comment

Choose a reason for hiding this comment

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

General comment - should we also report progress on Push?

The AI caught what is likely a logical bug in the test as well, which should be fixed or explained before merging.

… PullEnded.

Added Exception and ServiceResponse to SynchronizationEventArgs.
Fixed bug in SynchronizationProgress_Event_Works test. Initialization of eventFired was wrong.
SynchronizationProgress_Event_Works added tests for start and end.
Added test PullAsync_List_FailedRequest_SynchronizationEventWorks
Added Synchronization events for push operations and test for it.
@StefanCuypers
Copy link
Contributor Author

I've added pull start and end events and included Exception reporting in the end event, which could be used to have the info in request #383 .
I also added reporting on push operations. Used the same event and adjusted it since the nature of push and pull is a bit different. On pull we report per entity type, where on push we do not since pushes work historicaly rather than by entity type.
And I fixed the test bug and added tests for Exception reporting on pull and for the push event reporting.

@StefanCuypers
Copy link
Contributor Author

@adrianhall I don't know why the Create_CanRoundtrip_Types test suddenly fails and worked before. But I found the cause of the failure. If I add a DateTimeValue = new DateTime(2025, 6, 30, 9, 5, 12) it seems to work fine.
Cause is that if I do not set DateTimeValue it defaults to 1/01/0001 0:00:00 with kind = unspecified. The kind = unspecified is interpreted as UTC time so sent to server as 0001-01-01T00:00:00.000Z. The AddAsync call then returns this correctly as 0001-01-01T00:00:00.000Z back to the client, but there the DateTimeConverter class does a DateTime.Parse() which converts the DateTime to local time kind and adjust the time to the local time zone.
The BeEquivalentTo call then compares the original datetime in unspecified timezone with the new one in local time zone and fails because it does not seem to look at the datetime kind.
Not sure if this needs fixing and how to fix it.

… side to handle 'default' DateTime correctly.
@StefanCuypers
Copy link
Contributor Author

I would propose to fix the Create_CanRoundtrip_Types failure for the specific case of a default DateTime to ensure 'default' is retained round-trip. With this fix 'default' is retained and any other DateTime is converted to local DateTime on round-trip as it was in previous versions.
The change adjusts the DateTimeConverter on client and on server side.
I've adjusted the PR for this.

…gs to ItemsTotal.

Made DateTimeConverter on server Except again if input is empty.
Optimised DataTimeConverter to avoid double parsing.
@StefanCuypers
Copy link
Contributor Author

@adrianhall : the test seems to fail on a timeout on QueueHandler_WithThreads_EnqueueRange(8). If I run this test on my PC it runs in 6sec which is well within the 10s limit. Maybe this was a temporary issue on the test servers? Could you rerun the test?

@adrianhall
Copy link
Collaborator

There is an issue about "flaky tests" - this is one of those cases where the underlying system can decide to go slow and we are left at its mercy. The only way to resolve it is to perhaps retry the test a few times, or extend the timeout.

There is also a common problem where the database container we are using has clock drift from the main test suite. This is likely not the case here though since I believe it's all in the client, so we aren't worried about the timestamps on the server side.

@adrianhall adrianhall requested review from Copilot and adrianhall July 1, 2025 19:48
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Adds a progress-reporting event to the offline sync APIs, wires it through pull and push operations, and verifies it with new tests.

  • Introduces SynchronizationProgress event and SynchronizationEventArgs in OfflineDbContext
  • Emits start/fetch/commit/end events in PullOperationManager and push item/start/end events in OperationsQueueManager
  • Adds comprehensive tests for pull- and push-progress events

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
tests/CommunityToolkit.Datasync.TestCommon/TestData/Movies.cs Added new movie fixtures (Dune, DrNo)
tests/CommunityToolkit.Datasync.Client.Test/Offline/OfflineDbContext_Tests.cs New tests for SynchronizationProgress during pull and push
src/CommunityToolkit.Datasync.Server.Abstractions/Json/DateTimeConverter.cs Enhance Read to preserve default and convert UTC to local
src/CommunityToolkit.Datasync.Client/Serialization/DateTimeConverter.cs Mirror server logic in client converter
src/CommunityToolkit.Datasync.Client/Offline/SynchronizationEventArgs.cs Define SynchronizationEventType enum and SynchronizationEventArgs
src/CommunityToolkit.Datasync.Client/Offline/OperationsQueue/OperationsQueueManager.cs Emit PushStarted, PushItem, and PushEnded events
src/CommunityToolkit.Datasync.Client/Offline/Operations/PullOperationManager.cs Emit PullStarted, ItemsFetched, ItemsCommitted, PullEnded events
src/CommunityToolkit.Datasync.Client/Offline/OfflineDbContext.cs Add SynchronizationProgress event and SendSynchronizationEvent
Comments suppressed due to low confidence (2)

src/CommunityToolkit.Datasync.Server.Abstractions/Json/DateTimeConverter.cs:23

  • Consider adding unit tests to verify that Read correctly handles default dates and converts UTC strings to local time.
        DateTime utc = DateTime.Parse(reader.GetString() ?? "", CultureInfo.InvariantCulture, DateTimeStyles.AdjustToUniversal | DateTimeStyles.AssumeUniversal);

src/CommunityToolkit.Datasync.Client/Serialization/DateTimeConverter.cs:30

  • Add unit tests for the client converter path to ensure default DateTime values are not shifted and UTC values convert properly.
            DateTime utc = DateTime.Parse(token, CultureInfo.InvariantCulture, DateTimeStyles.AdjustToUniversal | DateTimeStyles.AssumeUniversal);

… seconds instead of 1. This reduces the risk of thread reusage which could cause the distinct threads count to fail.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide progress report on pull operations
3 participants