Skip to content

Change tests to accomodate non blocking regular sync#584

Closed
MatMaul wants to merge 3 commits intomainfrom
mv/non-ll-sync-fast-join
Closed

Change tests to accomodate non blocking regular sync#584
MatMaul wants to merge 3 commits intomainfrom
mv/non-ll-sync-fast-join

Conversation

@MatMaul
Copy link
Contributor

@MatMaul MatMaul commented Jan 17, 2023

No description provided.

Copy link
Contributor

@DMRobertson DMRobertson left a comment

Choose a reason for hiding this comment

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

Some nitpicks about naming aside, I'm happy if CI is.

@MatMaul MatMaul requested a review from DMRobertson January 17, 2023 15:08
Copy link
Contributor

@DMRobertson DMRobertson left a comment

Choose a reason for hiding this comment

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

I think we're missing a test case here.

Suppose that we

  • we an do incremental, non-lazy sync, getting since_token 1.
  • we partially join a room.
  • we incremental, nonlazy sync from since_token 1 and get since_token 2.
    • The response here should not include the room. Is this tested?
  • someone sends a message to the room.
  • We incremental, nonlazy sync from since_token 2 and get since_token 3.
    • The response here should not include the room. Is this tested?
  • The resync completes.
    • We incremental, nonlazy sync from since_token 3 and get since_token 4.
    • The response here should include the room. I think this is tested?

(I think in this diff we only cover partial joins with an initial, nonlazy sync?)


// initial sync shouldn't include the room yet, but still return immediatly
response, nextBatch := alice.MustSync(t, client.SyncReq{
TimeoutMillis: "10000",
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure this adds anything since MustSync uses a timeout of 1000 by default:

query := url.Values{
"timeout": []string{"1000"},
}

I also think I got myself confused when writing up the comment last night.

I've had a go at this in #588.

Copy link
Contributor

Choose a reason for hiding this comment

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

MV: this isn't addressing the comment of dmr above, this is about the notifier changes (see synapse PR)

@DMRobertson
Copy link
Contributor

Superseded by #588

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.

2 participants