Skip to content
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

Tests for transaction ID semantics #622

Merged
merged 7 commits into from
Apr 13, 2023

Conversation

hughns
Copy link
Member

@hughns hughns commented Mar 4, 2023

This adds Client-Server API tests relation to transaction identifiers:

  1. That requests are idempotent within a single client session
  2. That transaction ID can be re-used across client sessions
  3. That the transaction ID is present in /sync response to deal with "local echo"

Signed-off-by: Hugh Nimmo-Smith hughns@element.io

@hughns hughns force-pushed the hughns/txnid-semantics branch 2 times, most recently from 16054c8 to 49d0556 Compare March 4, 2023 08:57
@hughns hughns marked this pull request as ready for review March 4, 2023 23:20
@hughns hughns requested review from a team and kegsay as code owners March 4, 2023 23:20
@hughns hughns changed the title Test for transaction ID semantics Tests for transaction ID semantics Mar 4, 2023
internal/client/client.go Show resolved Hide resolved
tests/csapi/txnid_test.go Show resolved Hide resolved
tests/csapi/txnid_test.go Outdated Show resolved Hide resolved
tests/csapi/txnid_test.go Outdated Show resolved Hide resolved
internal/client/client.go Outdated Show resolved Hide resolved
hughns and others added 2 commits March 6, 2023 19:17
@hughns hughns force-pushed the hughns/txnid-semantics branch from 4ff84ab to b6e1929 Compare March 6, 2023 19:17
@hughns hughns requested a review from kegsay March 9, 2023 18:03
@hughns hughns force-pushed the hughns/txnid-semantics branch from 12ca5a1 to 67f7d14 Compare March 9, 2023 18:42
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.

Other than the error messages I pointed out, I think I only have questions and comments rather than hard changes:

internal/client/client.go Outdated Show resolved Hide resolved
internal/client/client.go Outdated Show resolved Hide resolved
tests/csapi/txnid_test.go Show resolved Hide resolved
tests/csapi/txnid_test.go Outdated Show resolved Hide resolved
tests/csapi/txnid_test.go Outdated Show resolved Hide resolved
tests/csapi/txnid_test.go Show resolved Hide resolved
hughns and others added 2 commits April 12, 2023 09:55
Co-authored-by: David Robertson <david.m.robertson1@gmail.com>
@hughns hughns requested a review from DMRobertson April 13, 2023 17:38
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.

LGTM assuming CI is happy!

@DMRobertson DMRobertson merged commit 9f43aa2 into matrix-org:main Apr 13, 2023
@hughns hughns deleted the hughns/txnid-semantics branch April 18, 2023 17:12
hughns added a commit to sandhose/complement that referenced this pull request Apr 18, 2023
* Test for transaction ID semantics

* Update internal/client/client.go

Co-authored-by: kegsay <kegsay@gmail.com>

* Incorporate review feedback

* Refactor for legibility

* Clarify that conduit doesn't pass idempotency tests

* Apply suggestions from code review

Co-authored-by: David Robertson <david.m.robertson1@gmail.com>

* Incorporate review feedback

---------

Co-authored-by: kegsay <kegsay@gmail.com>
Co-authored-by: David Robertson <david.m.robertson1@gmail.com>
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.

3 participants