-
Notifications
You must be signed in to change notification settings - Fork 448
Add in-repo Complement tests #19406
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
base: develop
Are you sure you want to change the base?
Add in-repo Complement tests #19406
Conversation
| env: | ||
| POSTGRES: ${{ (matrix.database == 'Postgres') && 1 || '' }} | ||
| WORKERS: ${{ (matrix.arrangement == 'workers') && 1 || '' }} | ||
| TEST_ONLY_IGNORE_POETRY_LOCKFILE: 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just aligning to what we already do in .github/workflows/tests.yml
The difference is the TEST_ONLY_IGNORE_POETRY_LOCKFILE here
| env: | ||
| POSTGRES: ${{ (matrix.database == 'Postgres') && 1 || '' }} | ||
| WORKERS: ${{ (matrix.arrangement == 'workers') && 1 || '' }} | ||
| TEST_ONLY_SKIP_DEP_HASH_VERIFICATION: 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just aligning to what we already do in .github/workflows/tests.yml
The difference is the TEST_ONLY_SKIP_DEP_HASH_VERIFICATION here
| - name: Run in-repo Complement Tests | ||
| id: run_in_repo_complement_tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding a new CI step to to run the in-repo Complement tests
Same as other step but we have --in-repo. I wish there was a way to just run everything as a single command/step but it doesn't seem very possible since any kind of finagling the tests together results in directory ... outside main module or its selected dependencies
| enable: | ||
| - gofmt | ||
| - goimports | ||
| - golines |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same config that we have in element-hq/synapse-small-hosts -> complement/.golangci.yml
https://github.com/element-hq/synapse/actions/runs/21303838770/job/61327615707?pr=19406 ``` +++ dirname synapse/scripts-dev/complement.sh ++ realpath synapse/scripts-dev/../complement realpath: synapse/scripts-dev/../complement: No such file or directory + in_repo_test_suite= ```
Add in-repo Complement tests. This is useful so we can test Synapse specific behaviors like our admin API.
Complement calls these "out-of-repo" tests but it's a bit of a misnomer once they're in your project.
There has been previous desire for this kind of thing but this is spawning from wanting to have some tests for our purge history admin API. There are some Sytest tests (
matrix-org/sytest->tests/48admin.pl#L91-L618) for this already but I'd much rather work in Complement instead of Sytest. I'm wanting these tests to ensure that our newevent-cacherust app for Synapse Pro doesn't break these kind of erasure features (https://github.com/element-hq/synapse-rust-apps/issues/366 and https://github.com/element-hq/synapse-rust-apps/issues/153).Interestingly, there is already
matrix-org/complement->tests/csapi/admin_test.go(added in matrix-org/complement#322) in the Complement repo iteslf that tests the/_synapse/admin/v1/send_server_noticeendpoint but it's a bit of an interesting case as Dendrite also supports this endpoint. I don't think it's good practice to continually shove in more and more Synapse-specific behavior into the Complement repo itself.We already have success with other out-of-repo tests for projects like the SBG, TI-Messenger Proxy, and our Synapse Pro for small hosts.
Pull Request Checklist
EventStoretoEventWorkerStore.".code blocks.