tests: Add initial coverage for check_system_user_group_allowed_for_setting. #36978
Conversation
arshalik1111-design
force-pushed
the
add-user-groups-tests
branch
from
2a2b316 to
a0e4ad0
Compare
|
Thanks! Please go through our guide on submitting a pull request to learn how to present your proposed changes to Zulip, so that your work can be reviewed. |
arshalik1111-design
force-pushed
the
add-user-groups-tests
branch
4 times, most recently
from
6b4aeb6 to
e68a7de
Compare
|
Ready for review! I have squashed the commits into a single atomic commit ( As noted in the description, I focused this PR on testing the structure and flags logic to ensure stability, while omitting the display formatter tests to avoid dependencies on the legacy locale setup. All tests are passing. |
|
Please fill out the self-review checklist, which is for some reason missing from the PR description. Your commit message still isn't formatted correctly. |
|
You can ping @shubham-padia for maintainer review when ready. |
…etting Add comprehensive unit tests for user_groups.js, covering: - Group lifecycle (add_user_group, remove_user_group) - Member and subgroup management - Property updates (update_user_group_properties) - Getters, queries, and helper functions (is_empty_group, misc_helpers) - Structure and flags logic Fixes zulip#33208
arshalik1111-design
force-pushed
the
add-user-groups-tests
branch
from
e68a7de to
217994a
Compare
|
@shubham-padia This is ready for maintainer review. |
shubham-padia
left a comment
shubham-padia
left a comment
There was a problem hiding this comment.
I left a comment on the first test, but my recommendation here is to split each new test in a new commit, then go through each commit yourself one-by-one, study the code your test is testing against and make sure all testable scenarios are covered in that test.
Go through the original code slowly, see all the possible scenarios and then add comprehensive tests for it.
| ), | ||
| true, | ||
| ); | ||
| }); |
There was a problem hiding this comment.
For this test, we need to check allowed_system_groups, realm_waiting_period_threshold and all scenarios of for_new_settings_ui (it being both true and false for different if blocks.
|
@arshalik1111-design If you're forfeiting the issue, closing the PR is fine. If you plan to make the changes, please don't close and open a different PR. You can force push any changes you want to this PR itself. |
4 participants
Summary This PR increases test coverage for
user_groups.jsby adding unit tests for group structure queries and membership flags (e.g.,is_setting_group_empty,is_user_in_setting_group).Technical Details
Restored System Groups: I explicitly restored standard system groups (Admins, Moderators, etc.) in
user_groups.test.cjs. This ensures backward compatibility with existing legacy tests that rely on these groups being present in the global scope.New Tests: Added the
structure_and_flagstest block to verify logic for checking empty groups and user membership.Scope Note: I originally intended to test
format_group_list(display settings). However, I have omitted those specific tests in this PR because the function requires complexuser_settingslocale initialization that conflicted with the legacy test runner. I prioritized robust logic coverage for structure and flags.Manual Testing
Ran
./tools/test-js-with-node user_groupslocally.Verified 100% pass rate (All checks green).
Fixes Fixes #33208
Visuals
There are no UI/Visual changes.
Self-review checklist
Communicate decisions, questions, and potential concerns
Individual commits are ready for review (see commit discipline)
Completed manual review and testing of the following