Skip to content

E2E: QA Added acceptance tests for user management from user group#22919

Open
nhudinh0309 wants to merge 11 commits into
mainfrom
v18/QA/manage-users-from-user-group-tests
Open

E2E: QA Added acceptance tests for user management from user group#22919
nhudinh0309 wants to merge 11 commits into
mainfrom
v18/QA/manage-users-from-user-group-tests

Conversation

@nhudinh0309

Copy link
Copy Markdown
Contributor

This PR adds acceptance test for user management from user group feature implemented in this PR: #22215

@claude

claude Bot commented May 21, 2026

Copy link
Copy Markdown

Claude finished @nhudinh0309's task in 2m 26s —— View job


PR Review

Target: origin/main · Based on commit: 61dfe4dd · Classified as: Test-only

Adds 10 E2E acceptance tests covering user management from the user group workspace (view, add single/multiple, remove, persistence) along with 5 new UI helper methods in UserGroupUiHelper and a package.json script change.


Important

  • package.json:27: The smokeTest script was narrowed from --grep "@smoke" (runs ALL @smoke-tagged tests across DefaultConfig) to DefaultConfig/Users/ManageUsersFromGroup (runs only this one file). There are dozens of other spec files with @smoke tags (DocumentType, MediaType, Templates, Scripts, etc.) that would be silently dropped from the smoke run. This appears unintentional — it should be reverted to the original grep, and if a targeted script for this file is needed, a separate entry (e.g. "testManageUsersFromGroup") should be added instead. Fix this →

Suggestions

  • ManageUsersFromGroup.spec.ts:65,88,113,135,155-158,178,181,198,225: waitForTimeout(ConstantHelper.wait.medium) is used after picker confirmation and after remove confirmation throughout the tests. While this pattern exists in the codebase, waiting for a specific API response (similar to clickSaveButtonAndWaitForUserGroupToBeCreated) would be more reliable. Consider intercepting the user-update API call instead of relying on a 1 s fixed delay — the existing waitForResponseAfterExecutingPromise helper could be reused.

  • ManageUsersFromGroup.spec.ts:37-38: The 'can view existing users in a user group' test asserts both users are visible but doesn't verify the count. A third unexpected user in the group would not be caught. Adding expect(await umbracoUi.userGroup.getUsersInGroupCount()).toBe(2) would tighten the assertion.

  • ManageUsersFromGroup.spec.ts:56: writersUserGroupName = 'Writers' is declared at module scope and used as a seed-data anchor across 7 tests. This is a common pattern in the acceptance suite, but it does make the tests load-bearing on that seed group existing. A brief inline comment explaining the dependency on seed data (// seed data – "Writers" group is always present) might help future readers.


Approved with Suggestions for improvement

Good to go, but please address the smokeTest script regression — it silently drops all other smoke tests from the smoke run. The test logic itself is well-structured with clear AAA sections, good edge case coverage, and appropriate @smoke/@release tagging.

@andr317c andr317c left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had a few comments

@andr317c andr317c left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had some comments

Comment thread tests/Umbraco.Tests.AcceptanceTest/lib/helpers/UserGroupUiHelper.ts Outdated
andr317c and others added 3 commits June 25, 2026 13:35
…rom-user-group-tests

# Conflicts:
#	tests/Umbraco.Tests.AcceptanceTest/lib/helpers/UserGroupUiHelper.ts
@andr317c andr317c enabled auto-merge (squash) June 25, 2026 12:47
@sonarqubecloud

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants