Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

User access group test #14

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

User access group test #14

wants to merge 14 commits into from

Conversation

nk2136
Copy link
Contributor

@nk2136 nk2136 commented Nov 14, 2024

Test scenarios for User Access Group test -

  1. Create group and add an employee
  2. Import group from another leaf site and delete it
  3. System admin search
  4. View user group history

@nk2136 nk2136 force-pushed the user-access-group-test branch 2 times, most recently from 3994d0e to c56c1b2 Compare November 14, 2024 20:59
Copy link
Collaborator

@mgaoVA mgaoVA left a comment

Choose a reason for hiding this comment

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

image

Before requesting reviews, all tests should pass using this procedure:

  1. Check that you are using the correct Git branches, and they are up to date
    • LEAF -> "master" branch
    • LEAF-Automated-Tests -> your working branch
  2. Navigate to the Development dashboard (https://host.docker.internal/)
  3. Click on "API tester", and wait for it to complete
  4. Click on "End-to-End tests (Playwright)"

@nk2136 nk2136 force-pushed the user-access-group-test branch from c56c1b2 to 17e27de Compare November 15, 2024 05:48

const saveButton = page.getByRole('button', { name: 'Save' });
await saveButton.click();

Copy link
Contributor

Choose a reason for hiding this comment

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

This does not error on repeat runs, but I'm concerned that the test does not reflect it only being possible to create a group of a specific name once (even if it's subsequently deleted).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Elaborating on the concern: Tests that involve creating and validating entries must not use fixed values. Subsequent runs of this test fail to validate intended functionality: whether the user can create groups. In this test, the second run actually skips the group creation because the group already exists (which would normally trigger an error).

Instead of:

  await groupTitle.fill('New Test Group 0');

A more complete solution would be:

let randNum = Math.random();
let uniqueText = `New Test Group ${randNum}`;

await groupTitle.fill(uniqueText);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Copy link
Collaborator

@mgaoVA mgaoVA left a comment

Choose a reason for hiding this comment

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

  1. Please update the PR description to reflect changes in scope. Since tests were removed, the description should reflect it.

  2. When naming tests, try to think about the procedure from the end-user's perspective. For example, instead of "Validate group creation and an employee addition", a more concise label would be "create group and add employee". It's not necessary to explain that the test is doing validation -- this is implied since all tests are fundamentally validating functionality.

  3. The "waitFor" in this pattern is redundant because the second line automatically waits:

  await group.waitFor();
  await group.click();
  1. To improve repeatability, it would be prudent to expand certain tests. For example, when importing a group, it's easier to also remove it within the same test so that subsequent runs fully exercise the intended functionality. Currently these tests trigger an error, but it doesn't show up because it's based on an asynchronous transaction, and the test's locators end up skipping it.

@nk2136 nk2136 force-pushed the user-access-group-test branch from 2203f31 to cb12d75 Compare December 5, 2024 18:36
@nk2136 nk2136 force-pushed the user-access-group-test branch from cb12d75 to 73acc32 Compare December 6, 2024 07:59
@nk2136 nk2136 force-pushed the user-access-group-test branch from 1c119c8 to 8e2fc6a Compare January 9, 2025 15:49
@nk2136 nk2136 force-pushed the user-access-group-test branch from 8e2fc6a to 2767337 Compare January 9, 2025 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants