github/penpot
1
0
Fork 0
mirror of https://github.com/penpot/penpot.git synced 2026-11-10 20:10:37 +00:00
Code Issues Packages Projects Releases Wiki Activity

🐛 Make the revocation tests deterministic

Browse Source 
Third review round on PR #12189 (pabloalba).

Both tests took access away through a command that announces, so the
live watcher could close the subscriptions before the test looked:

- revocation-closes-subscriptions-that-are-no-longer-authorized
  asserted an exact count of 2 closes after delete-team-member, and
  that count reads 0 whenever the watcher got there first. It now
  removes the member with delete-team-membership!, the raw row delete
  that publishes nothing, and asserts both subscriptions are still
  open before calling revalidate-profile-subscriptions.
- team-event-closes-every-member-subscription ran delete-team and then
  published a second notify-team-permissions-changed, so a missing
  announcement in delete-team still passed. It now soft-deletes the
  team row (sql:team-members joins team_profile_rel and profile only,
  so the watcher still resolves every member), asserts nothing closed,
  and drives one synthetic team event.

The command paths stay covered by the announce and end-to-end tests
for delete-team-member and delete-team. The file split the reviewer
suggested stays the optional follow-up, tracked outside this PR and
waiting for the merge.

Closes #12155

AI-assisted-by: deepseek-v4.1-flash
This commit is contained in:
Andrey Antukh 2026-11-10 17:16:28 +00:00
parent 05f388c091
commit 6f17c27f52
2 changed files with 23 additions and 13 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
.serena/memories/backend/websocket-subscriptions.md
View File

@ -32,7 +32,7 @@ Which mutations announce: `delete-team-member`, `leave-team`, `update-team-membe
## Tests
`backend/test/backend_tests/http_websocket_test.clj` drives the real Redis-backed msgbus, so the revocation tests are end-to-end: register the connection in the real registry, run the real command, wait for the subscription to drop, then assert no content arrives. A connection built without `register-connection` is invisible to the watcher, so the watcher-based tests must go through `open-registered-connection`. Interval tests shorten the config via `with-redefs` on `cf/get` rather than waiting minutes. `with-clean-registry` runs as an `:each` fixture, so no test can leak a live relay loop into the next one. `captured-events` forwards the keyword args through to the real `mbus/pub!` — rebuilding them positionally publishes to the topic `:message` with a `nil` payload and silently untests every other notice. Interval tests take access away with a direct row delete, never with a command: a command announces, so the test would pass even with the interval removed. The resubscribe-counting test is the one that proves finished relays stop polling. The soft-delete + org-owner re-check tests pair a direct row soft-delete with `with-redefs` on `nitrate/organization-owner-of-team?`: before the existence gate, the org-owner fallback kept the subscription open on a deleted resource. The soft-deleted-project/team-closes-org-owner-file-subscription tests pin the root fix in `app.rpc.permissions` (`resolve-team-id` refuses dead file/project/team for every caller of the fallback). `watcher-consumes-revocation-events-from-the-bus` takes access away with a direct row delete and drives the watcher with a synthetic bus event — never a command and never a second `notify-*` — so the "still subscribed" assertion cannot race an after-commit hook and a broken command announcement cannot mask a broken watcher. `moving-a-project-closes-source-team-subscriptions` pins the move-project asymmetry: the file subscription drops, the source-team subscription survives (the member is still a member).
`backend/test/backend_tests/http_websocket_test.clj` drives the real Redis-backed msgbus, so the revocation tests are end-to-end: register the connection in the real registry, run the real command, wait for the subscription to drop, then assert no content arrives. A connection built without `register-connection` is invisible to the watcher, so the watcher-based tests must go through `open-registered-connection`. Interval tests shorten the config via `with-redefs` on `cf/get` rather than waiting minutes. `with-clean-registry` runs as an `:each` fixture, so no test can leak a live relay loop into the next one. `captured-events` forwards the keyword args through to the real `mbus/pub!` — rebuilding them positionally publishes to the topic `:message` with a `nil` payload and silently untests every other notice. Interval tests take access away with a direct row delete, never with a command: a command announces, so the test would pass even with the interval removed. The same rule covers the two counting/fan-out tests: `revocation-closes-subscriptions-that-are-no-longer-authorized` takes access away with `delete-team-membership!` (raw row delete, publishes nothing) because a command announcement lets the live watcher close both subscriptions before the test's own `revalidate-profile-subscriptions` call, and its `(= 2 …)` count then reads 0; `team-event-closes-every-member-subscription` soft-deletes the team row (`sql:team-members` joins `team_profile_rel` and `profile` only, so the watcher still resolves every member) and asserts nothing closed before one synthetic `notify-team-permissions-changed`, leaving the command path to `deleting-a-team-announces-once` and `deleting-a-team-closes-open-subscriptions`. The resubscribe-counting test is the one that proves finished relays stop polling. The soft-delete + org-owner re-check tests pair a direct row soft-delete with `with-redefs` on `nitrate/organization-owner-of-team?`: before the existence gate, the org-owner fallback kept the subscription open on a deleted resource. The soft-deleted-project/team-closes-org-owner-file-subscription tests pin the root fix in `app.rpc.permissions` (`resolve-team-id` refuses dead file/project/team for every caller of the fallback). `watcher-consumes-revocation-events-from-the-bus` takes access away with a direct row delete and drives the watcher with a synthetic bus event — never a command and never a second `notify-*` — so the "still subscribed" assertion cannot race an after-commit hook and a broken command announcement cannot mask a broken watcher. `moving-a-project-closes-source-team-subscriptions` pins the move-project asymmetry: the file subscription drops, the source-team subscription survives (the member is still a member).
`revocation-watcher-schema-resolves-every-ref` covers the `::revocation-watcher` params schema, which no other test compiles. It existed broken for a while: the three injected keys were written as bare keywords, which is a valid malli *property* position and not a key, so the schema raised `:malli.core/invalid-schema` — invisible unless `PENPOT_FLAGS=enable-backend-asserts` was set, and a crash of the whole system build rather than one test (see `mem:backend/testing` on the flag).

34
backend/test/backend_tests/http_websocket_test.clj
View File

@ -463,7 +463,7 @@
team-id profile-id]))
(t/deftest revocation-closes-subscriptions-that-are-no-longer-authorized
(let [{:keys [owner editor team file]} (editor-in-team! 1 2)]
(let [{:keys [editor team file]} (editor-in-team! 1 2)]
(let [conn (-> (open-registered-connection (:id editor))
(subscribe :subscribe-file {:file-id (:id file)})
@ -473,12 +473,15 @@
(t/is (= (:id file) (subscribed-file conn)))
(t/is (= (:id team) (subscribed-team conn))))
(t/testing "the member is removed from the team"
(let [result (th/command! {::th/type :delete-team-member
::rpc/profile-id (:id owner)
:team-id (:id team)
:member-id (:id editor)})]
(t/is (th/success? result))))
;; A direct row delete fires no command and publishes no
;; announcement, so the explicit revalidation below is the only
;; thing that can close these subscriptions and the count is
;; deterministic. A command would announce, and the live watcher
;; could close them before the count is taken.
(t/testing "the member is removed straight over SQL, so nothing announces it"
(delete-team-membership! (:id team) (:id editor))
(t/is (= (:id file) (subscribed-file conn)))
(t/is (= (:id team) (subscribed-team conn))))
(t/testing "revalidation closes the file and the team subscription"
(t/is (= 2 (ws/revalidate-profile-subscriptions
@ -698,7 +701,7 @@
events)))))
(t/deftest team-event-closes-every-member-subscription
(let [{:keys [owner editor team file]} (editor-in-team! 1 2)
(let [{:keys [editor team file]} (editor-in-team! 1 2)
other (th/create-profile* 3 {:is-active true})]
(th/create-team-role* {:team-id (:id team)
:profile-id (:id other)
@ -712,11 +715,18 @@
conn2 (-> (open-registered-connection (:id other))
(subscribe :subscribe-file {:file-id (:id other-file)}))]
(th/command! {::th/type :delete-team
::rpc/profile-id (:id owner)
:id (:id team)})
;; Same shape as the watcher test above: a direct soft-delete
;; publishes nothing, so this test owns the fan-out of one event
;; and the announcement of `delete-team` is left to
;; `deleting-a-team-announces-once`. A second notify here would
;; make the test pass even if that announcement disappeared.
(db/update! th/*system* :team {:deleted-at (ct/now)} {:id (:id team)})
(t/testing "the announcement reaches the watcher off the bus"
(t/testing "nothing closes until the event arrives"
(t/is (some? (subscribed-file conn)))
(t/is (some? (subscribed-file conn2))))
(t/testing "one team event reaches the watcher off the bus"
(ntf/notify-team-permissions-changed th/*system* (:id team))
(t/is (wait-until #(nil? (subscribed-file conn)) 3000))
(t/is (wait-until #(nil? (subscribed-file conn2)) 3000)))))))