tests: Consolidate stream access tests into table-driven structure. #37024
Conversation
|
Thanks! Please fill out the self-review checklist, which is for some reason missing from the PR description. Your PR will also need to pass tests. |
|
Hello @zulip/server-testing members, this pull request was labeled with the "area: testing-coverage" label, so you may want to check it out! |
I’ve now added the full self-review checklist to the PR description, as requested. |
|
Please clean up your commit history and post again to request a review. See here for guidelines. |
Git-HimanshuRathi
force-pushed
the
issue-33600-stream-data-test-refactor
branch
from
ccb0810 to
7efe85a
Compare
|
I’ve cleaned up the commit history using an interactive rebase and force-pushed a single coherent commit, following the guidelines that you've shared . All tests are passing. Please let me know if anything else is needed before review. |
alya
added
maintainer review
|
@shubham-padia Could you please take a look at this one? |
shubham-padia
left a comment
shubham-padia
left a comment
There was a problem hiding this comment.
Had a brief look and posted some comments. Can you explain how did you make sure that all the previous test cases are included in the new test. I could find some missing cases on a brief look, so the first thing to ensure here is that we don't lose any test cases during this change.
web/tests/stream_data.test.cjs
Outdated
| assert.equal(stream_data.has_metadata_access(social), true); | ||
| user: {is_admin: false, is_guest: false}, | ||
| spectator: false, | ||
| expect: {meta: true, content: true, preview: false}, |
There was a problem hiding this comment.
Let's use the full names here, e.g has_content_access, has_metadata_access and can_preview so it's clear what we are testing just from the test case.
web/tests/stream_data.test.cjs
Outdated
| ]; | ||
|
|
||
| run_test("stream access table-driven test", ({override}) => { | ||
| for (const tc of access_cases) { |
There was a problem hiding this comment.
let's use test_case instead of tc
There was a problem hiding this comment.
Why are we removing all these testcases for can_access_stream_email and shortening the test?
There was a problem hiding this comment.
Restored can_access_stream_email test
The full original test is now restored with all cases:
Archived stream → false
Spectator → false
User in permission group → true
Bot in permission group → true
Unknown user → false
Guest → false
|
|
||
| override_rewire(stream_data, "has_content_access", () => true); | ||
| assert.equal(stream_data.can_preview(social), true); | ||
| social.history_public_to_subscribers = false; |
There was a problem hiding this comment.
Same here with can_toggle_subscription, why are we shortening the test?
There was a problem hiding this comment.
Restored
can_toggle_subscription
All original cases are now included:
Spectator → false
Subscribed user → true
Guest subscribed → true (can unsubscribe)
Unsubscribed public channel → true
Unsubscribed private channel → false
Admin on private → false
User in can_add_subscribers_group → true
User in can_subscribe_group → true
| const social = { | ||
| subscribed: true, | ||
| color: "red", | ||
| name: "social", |
There was a problem hiding this comment.
We don't have a test for this and can_subscribe_group
There was a problem hiding this comment.
Added a dedicated standalone test can_subscribe_group grants access that verifies users in can_subscribe_group get metadata, content, and preview access.
Also added coverage in stream access with group permissions test that explicitly tests both can_add_subscribers_group and can_subscribe_group with guest user behavior.
Git-HimanshuRathi
force-pushed
the
issue-33600-stream-data-test-refactor
branch
2 times, most recently
from
4c7d4b2 to
c221339
Compare
|
CI is currently failing during provisioning due to a 503 error while downloading shellcheck (tools/setup/install-shellcheck). |
This commit refactors the has_metadata_access, has_content_access, and can_preview tests into a single table-driven test suite, making it easier to compare behavioral differences across various stream configs. Changes: - Combined related tests into 12-case table-driven test - Uses full function names in expect objects (e.g., has_metadata_access) - Added tests for guest users, spectators, and group permissions - Added standalone tests for can_add_subscribers_group, can_subscribe_group, and can_administer_channel_group - Restored full can_toggle_subscription and can_access_stream_email tests Fixes zulip#33600.
Git-HimanshuRathi
force-pushed
the
issue-33600-stream-data-test-refactor
branch
from
c221339 to
76cb107
Compare
|
I compared the new tests against the original main branch using git diff main -- web/tests/stream_data.test.cjs to ensure no test cases were lost. The table-driven test now has 12 cases (up from 7), and all original standalone test logic has been preserved or expanded. All tests pass and linting is clean. |
|
Could you please take a look and let me know if there’s anything that needs to be changed ? |
4 participants
This PR refactors the stream access tests in
stream_data.test.cjsby replacing the separatehas_metadata_access,has_content_access, andcan_previewtest blocks with a single,cleaner table-driven test.
Why this change?
As described in issue #33600:
filter.test.cjs).This update applies that approach to the stream access tests.
What this PR does
"stream access table-driven test").has_metadata_access(stream)has_content_access(stream)can_preview(stream)This removes duplication and significantly improves readability and maintainability.
Test results
All relevant tests passed locally:
✔ ./tools/test-js-with-node web/tests/stream_data.test.cjs — passed
✔ No regressions in other JS tests
✔ Backend + webhook tests unaffected (no backend code touched)
Issue
Fixes: #33600
Self-review checklist