User Channel History Compaction tests - #497
Conversation
There was a problem hiding this comment.
Pull request overview
Adds QE coverage and specifications for Sync Gateway user access-history compaction.
Changes:
- Adds tests covering collections, roles, concurrency, and offline-client behavior.
- Documents test scenarios and expected endpoint behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Review summary |
|---|---|
tests/QE/test_user_access_history_compaction.py |
Critical: configure collection sync functions for channel membership. Moderate: use a continuous replicator or narrow the live-session assertions. |
spec/tests/QE/test_user_access_history_compaction.md |
Documents the user access-history compaction test scenarios. |
Suppressed comments (10)
spec/tests/QE/test_user_access_history_compaction.md:140
- The possessive is punctuated as
'frank's, which makes the quoted username malformed in this step. Rephrase it as “the access history for user 'frank'”.
5. Get user 'frank's access history and check that channel 'ROLE_CHAN' is absent (the
spec/tests/QE/test_user_access_history_compaction.md:199
- The possessive is punctuated as
'iris's, leaving the quoted username malformed. Rephrase the step as “the access history for user 'iris'”.
4. While that session is still open, compact channel 'A' out of user 'iris's access
history.
spec/tests/QE/test_user_access_history_compaction.md:310
- The possessive is punctuated as
'leo's, leaving both quoted usernames malformed in this step. Rephrase it as “the access of user 'leo'” and “that user's access history.”
5. While offline, revoke user 'leo's access to channel 'A', then compact channel 'A' out
of user 'leo's access history before the client reconnects.
tests/QE/test_user_access_history_compaction.py:517
- The result of the compaction is only dumped and never asserted, so this test still passes if the endpoint returns an empty/no-op result and the live-session scenario is not actually exercised. Assert that
Awas reported as compacted (and, ideally, that it is absent from the subsequent access-history GET) before relying on the laterBvisibility check.
compacted = await sg.compact_user_access_history(db_name, "iris", {"_default": {"_default": ["A"]}})
self._dump("compact_user_access_history/iris while session is live", compacted)
tests/QE/test_user_access_history_compaction.py:647
- This test never verifies that the
db1compaction succeeded; it only checks thatdb2was unaffected. A broken or no-op compaction would therefore pass becausedb2would still containAeither way. Assert that the response reportsAas compacted (and/or verifydb1history is empty) before checking isolation.
self.mark_test_step("Compact channel 'A' for that user on the first database only")
compacted = await sg.compact_user_access_history("db1", "shared_name", {"_default": {"_default": ["A"]}})
self._dump("compact_user_access_history/shared_name on db1", compacted)
tests/QE/test_user_access_history_compaction.py:350
- The test never verifies that
frankactually inheritsROLE_CHANbefore the role is updated. Ifadd_roleor the user'sadmin_rolesassignment were ignored, every later assertion (empty user history, empty user compaction, and a missing role endpoint) would still pass. Create a document inROLE_CHANand assert it is visible through a client authenticated asfrankbefore removing the role's access.
self.mark_test_step("Create user 'frank' assigned to role 'myrole', with no direct channel access of their own")
await sg.add_user(
db_name,
"frank",
password="pass",
admin_roles=["myrole"],
collection_access={"_default": {"_default": {"admin_channels": []}}},
)
tests/QE/test_user_access_history_compaction.py:470
- Any
CblSyncGatewayBadResponseErroris accepted as the expected concurrent loser, including unrelated 400/401/404 responses or a server error unrelated to the race. That makes this regression pass when the endpoint is malformed or unauthorized. Restrict the accepted status to the conflict-style codes used by the analogous document-compaction test (409/500).
assert isinstance(failure, CblSyncGatewayBadResponseError)
tests/QE/test_user_access_history_compaction.py:357
- The possessive is punctuated as
'frank's, which makes the quoted username malformed. Rephrase it as “the access history for user 'frank'” (or usefrank'swithout surrounding quotes).
"Get user 'frank's access history and check that channel 'ROLE_CHAN' is absent "
"(the history lives on the role's own principal record, not the user's)"
tests/QE/test_user_access_history_compaction.py:514
- The possessive is punctuated as
'iris's, leaving the quoted username malformed. Rephrase this as “the access history for user 'iris'” to make the step unambiguous.
"While that session is still open, compact channel 'A' out of user 'iris's access history"
tests/QE/test_user_access_history_compaction.py:705
- The possessive is punctuated as
'leo's, leaving the quoted username malformed in this test step. Rephrase it as “the access of user 'leo'” and refer to “that user's” history.
"While offline, revoke user 'leo's access to channel 'A', then compact channel 'A' out of "
"the user's access history before the client reconnects"
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Test bug fixes + prints for a potential bug Inverted the test, since it was failing as expected. Now its a regression test removed debug statements Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> ruff formatting removed debug statements ruff formatting Converted helpers into static funcs + calls
8627b26 to
7850e0a
Compare
| db_name = "db" | ||
|
|
||
| self.mark_test_step("Create a bucket and configure a Sync Gateway database on it") | ||
| await _setup_db(sg, cblpytest.clusters[0].sync_gateway_cluster, cbs, db_name, "data-bucket") |
There was a problem hiding this comment.
Every single-collection call site collapses to one line:
| await _setup_db(sg, cblpytest.clusters[0].sync_gateway_cluster, cbs, db_name, "data-bucket") | |
| await cblpytest.clusters[0].create_database(db_name, _ACCESS_TRACKING_CONFIG) |
Same at lines 140, 156, 181, 231, 360, 455, 499, 576, 611 and 699.
The mark_test_step text above stays accurate -- Cluster.create_database does create the bucket.
The two-database test at lines 657-658 needs a different bucket per database; DatabaseConfig is a pydantic model, so:
await cblpytest.clusters[0].create_database("db1", _ACCESS_TRACKING_CONFIG.model_copy(update={"bucket": "data-bucket-1"}))
await cblpytest.clusters[0].create_database("db2", _ACCESS_TRACKING_CONFIG.model_copy(update={"bucket": "data-bucket-2"}))| await _setup_db( | ||
| sg, cblpytest.clusters[0].sync_gateway_cluster, cbs, db_name, "data-bucket", extra_collections=["other"] | ||
| ) |
There was a problem hiding this comment.
The three tests that need an extra collection pass their config inline, the way #495 handled its one multi-collection test:
| await _setup_db( | |
| sg, cblpytest.clusters[0].sync_gateway_cluster, cbs, db_name, "data-bucket", extra_collections=["other"] | |
| ) | |
| await cblpytest.clusters[0].create_database( | |
| db_name, | |
| DatabaseConfig( | |
| bucket=_BUCKET, | |
| index=IndexConfig(num_replicas=0), | |
| scopes={ | |
| "_default": ScopeConfig( | |
| collections={ | |
| "_default": {"sync": _CHANNEL_SYNC_FUNCTION}, | |
| "other": {"sync": _CHANNEL_SYNC_FUNCTION}, | |
| } | |
| ) | |
| }, | |
| ), | |
| ) |
Same at lines 309 and 413.
| case from a user-history angle). Uses the shared `backfill_after_offline` helper | ||
| (`tests/shared/backfill_after_offline.py`) to simulate the offline window for both the |
There was a problem hiding this comment.
| case from a user-history angle). Uses the shared `backfill_after_offline` helper | |
| (`tests/shared/backfill_after_offline.py`) to simulate the offline window for both the | |
| case from a user-history angle). Uses `_one_shot_pull` helper to | |
| simulate the offline window -- the gap between the two one-shot pulls -- for both the |
| async def _setup_db( | ||
| sg: SyncGateway, | ||
| sg_cluster: SyncGatewayCluster, | ||
| cbs: CouchbaseServer, | ||
| db_name: str, | ||
| bucket_name: str, | ||
| extra_collections: list[str] | None = None, | ||
| ) -> None: |
There was a problem hiding this comment.
Can you pick up the changes from the stacked PR to remove this function?
| assert "A" in _channels(history) | ||
|
|
||
| self.mark_test_step("Create a document in channel 'B'") | ||
| await sg.update_documents(db_name, [DocumentUpdateEntry("doc_b", None, {"channels": ["B"]})]) |
There was a problem hiding this comment.
this I think will need wait_for_caching_feed=True. The compaction will work without the cahing feed but wait_for_all_documents will not.
| self.mark_test_step( | ||
| "As user 'bob', fetch all documents and check that the channel-'B' document is still visible" | ||
| ) | ||
| user_client = _user_client(sg, "bob", password) |
This PR is the exact same motive as #495, but focuses on User Channel History Compaction.
The tests are also focusing on the same, they are similar to Document CHC, but written from User CHC point of view.
There are a couple of regression tests, but mostly its Unit and Functional testcases which span across CBS-SGW-CBL and verify things from CBL point of view for users as well.
This PR relies on a file pushed in #495, since the backfill was used by the test in that file as well, but after discussion with @RIT3shSapata and @bbrks , that single test was removed, as seen in one recent commit in that PR.
Hence the file is in a different PR and is being used in a different PR.