test: more accurately simulate a remote note change

[marksvc]

This change is 

[codecov]

Codecov Report

❌ Patch coverage is 94.73684% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 82.19%. Comparing base (1c26d26) to head (25a8930).
⚠️ Report is 14 commits behind head on master.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...ientApp/src/xforge-common/test-realtime.service.ts	94.44%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3469      +/-   ##
==========================================
- Coverage   82.20%   82.19%   -0.02%     
==========================================
  Files         611      611              
  Lines       36433    36468      +35     
  Branches     6004     6008       +4     
==========================================
+ Hits        29951    29975      +24     
- Misses       5608     5611       +3     
- Partials      874      882       +8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[marksvc]

A unit test highlighted races, suspected incorrect attempts to simulate remote changes, and seeming data inconsistencies between queries and the documents they query.

This unit test problem

The test is described as responding to a remote update, but is applying the update as a local change.
The way the change is applied is not awaited, and raced against updating the query.

editor.component.spec.ts TestEnvironment.resolveNote races a call to noteDoc.submitJson0Op before calling realtimeService.updateQueryAdaptersRemote:

noteDoc.submitJson0Op(op => op.set(n => n.status, NoteStatus.Resolved));
this.realtimeService.updateQueryAdaptersRemote();

If we change the winner of the race by modifying the code so that submitJson0Op will finish before updateQueryAdaptersRemote starts, like so

    noteDoc
      .submitJson0Op(op => op.set(n => n.status, NoteStatus.Resolved))
      .then(() => {
        this.realtimeService.updateQueryAdaptersRemote();
      });

then editor.component.spec.ts test 'should remove resolved notes after a remote update' shows a failure, saying

Error: Expected 5 to equal 4.

(And it might also be meaningful to modify realtime-doc.ts submit() so the call to adapter.submitOp is awaited rather than raced.)

When the test calls noteDoc.submitJson0Op, we get into realtime-query.ts onChange and it changes the count from 5 to 4. But it is a local operation and so does not emit a remote change.
When the test does realtimeService.updateQueryAdaptersRemote, we get into realtime-query.ts onChange, but count is already 4, and it does not see a change to emit about.
One might argue that there was a remote change, as understood by memory-realtime-remote-store.ts MemoryRealtimeQueryAdapter.perfomQuery seeing that it remembers the remote data being different. (Though realtime-query.ts onChange 'remembers' the remote's up-to-date information.)

The test applies an op locally to resolve a note, but is titled as removing such notes after a remote update.

It is not a successful fix to the test to change editor.component.spec.ts resolveNote to specify source as false. If we do this, memory-realtime-remote-store.ts MemoryRealtimeDocAdapter.submitOp does call emitRemoteChange, causing editor.component.ts loadNoteThreadDocs to detect changes from noteThreadQuery (presumably remoteDocChanges$). But the note thread query RealtimeQuery.docs is not up-to-date for presumably editor.component.currentChapterNoteThreadDocs to read an updated list. (Because realtime-query.ts onChange has not been run.)
realtime-doc.ts submit calls RealtimeService.onLocalDocUpdate, even if source is remote. submit does not seem to be designed to apply "purely remote" ops.

Better simulation of remote change

A helper method now updates both doc data and query results before either are announced. The helper, TestRealtimeService.simulateRemoteChangeJson0Op, seeks to more accurately simulate a remote change during testing. Previously, "remote doc" data was being updated and announced before query results were updated, leading to the application examining outdated query results and failing the test.

This PR changes how the remote change simulation is done. Instead of calling noteDoc.submitJson0Op (which races to result in running realtime-query.ts onChange before RealtimeService.updateQueryAdaptersRemote is called), we call test-realtime.service.ts simulateRemoteChangeJson0Op. This helper method updates the doc adapter data, and updates the associated query adapters' docIds, before then emitting doc adapter .changes$ and remoteChanges$ and then query adapter remoteChanges$.

When query adapter remoteChanges$ is emitted, that gets into realtime-query.ts onChange where a change is found and RealtimeQuery.remoteChanges$ is emitted. Then in Editor.Component.loadNoteThreadDocs we adjust notes.

I can't help but wonder if I have not understood something in our codebase, and that's why I came to this solution. Or maybe I did find a legitimate timing inconsistency that our tests have being doing incorrectly. If so, is my solution in the right place to fix the behaviour?

(Question A) Should application code, and test code, assume that RealtimeDocAdapter.data and RealtimeQueryAdapter.docIds are in sync? That is, that neither is up-to-date before the other. And so if RealtimeQueryAdapter.remoteChanges$ fires, you could assume RealtimeDocAdapter.data is up-to-date, and if RealtimeDocAdapter.remoteChanges$ fires, you could assume RealtimeQueryAdapter.docIds is up to date.

General problem?

I am concerned that this could be a problem in other tests, either (1) incorrectly attempting to simulate remote changes by causing local changes, or (2) assuming query results will be up to date when we respond to doc data changes.

We may reveal potential race problems in our software by looking for places where RealtimeDoc.submitJson0Op() (or RealtimeDoc.submit()) is used to simulate a remote change in tests. (And even when submit source is set to false, it's still processed as a local change.)

RealtimeQuery.remoteDocChanges$

RealtimeQuery.onInsert takes a doc than is newly in the query result set. It subscribes to the doc remoteChanges$. If doc remoteChanges$ emits, RealtimeQuery responds by emitting RealtimeQuery.remoteDocChanges$. However, RealtimeQuery.onChanges might not have been run yet unless a RealtimeQueryAdapter.remoteChanges$ event was handled first. This could mean that RealtimeQuery.docs is not up-to-date when RealtimeQuery announces that a doc has changed.

So suppose you have code saying

realtimeQueryForMyOwnNotes.remoteDocChanges$.subscribe(() => {
  console.log('The data of one of your own notes has changed! Now your own notes are as follows:');
  console.log(realtimeQueryForMyOwnNotes.docs)
});

And suppose that a remote change happens that changes

someNote.owner = 'someone else';

In response to this,

• realtime-query.ts constructor RealtimeQueryAdapter.remoteChanges$'s listener should call onChange, and thus update RealtimeQuery.docs.
• RealtimeQuery.onInsert RealtimeDoc.remoteChanges$'s listener fires RealtimeQuery.remoteDocChanges$.
  And I assume they would be processed in an undefined order.

The realtimeQueryForMyOwnNotes.remoteDocChanges$ event handler would fire, and know that a doc in the query list had a data change. But when it asks the query for the documents (realtimeQueryForMyOwnNotes.docs), it could be getting an out of date list of documents that includes the note that your user no longer owns.

Am I thinking the right way about this?
(Question B) Should clients use RealtimeQuery.remoteDocChanges$ responsibly in light of this, or should clients expect that RealtimeQuery.docs is up-to-date when RealtimeQuery.remoteDocChanges$ fires?

I see that the TestRealtimeService.simulateRemoteChangesJson0Op method is emitting in the order that should cause the problem, so perhaps that should be adjusted if we keep it.