test: user tests continued [INFENG-455] #9214

JComins000 · 2024-04-22T16:00:01Z

Ticket

INFENG-455
INFENG-628

Description

All the remaining user tests from here
Tests are serial for now, but I'll make them parallel as a part of INFENG-658

Test Plan

Checklist

Changes have been manually QA'd
User-facing API changes need the "User-facing API Change" label.
Release notes should be added as a separate file under docs/release-notes/.
See Release Note for details.
Licenses should be included for new code which was copied and/or modified from any external code.

netlify · 2024-04-22T18:14:12Z

✅ Deploy Preview for determined-ui ready!

Name	Link
🔨 Latest commit	`a473f14`
🔍 Latest deploy log	https://app.netlify.com/sites/determined-ui/deploys/662bf11a4237350008d8827a
😎 Deploy Preview	https://deploy-preview-9214--determined-ui.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

JComins000 · 2024-04-22T18:34:25Z

webui/react/src/e2e/tests/userManagement.spec.ts

@@ -20,62 +22,11 @@ test.describe('User Management', () => {
    await expect(page).toHaveURL(userManagementPage.url);
  });

-  test.skip('Users table count matches admin page users tab', async ({ page }) => {


this moves to lower in the suite and we dont skip anymore

JComins000 · 2024-04-22T21:41:01Z

webui/react/playwright.config.ts

@@ -23,8 +23,8 @@ export default defineConfig({
  fullyParallel: !!process.env.CI,

  /* https://playwright.dev/docs/test-timeouts#global-timeout */
-  globalTimeout: process.env.PWDEBUG ? 0 : 5 * 60 * 1000, // 3 min unless debugging
-  timeout: 60000, // TODO [INFENG-628] Users page loads slow so we extend 5 minutes and 1 minute per test until we get an isolated backend
+  globalTimeout: process.env.PWDEBUG ? 0 : 500_000,


i was getting timeouts because my test suite is getting larger

codecov · 2024-04-22T21:43:43Z

Codecov Report

Attention: Patch coverage is 0% with 399 lines in your changes are missing coverage. Please review.

Project coverage is 37.92%. Comparing base (32b6ce6) to head (a473f14).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9214      +/-   ##
==========================================
- Coverage   44.71%   37.92%   -6.80%     
==========================================
  Files        1271      949     -322     
  Lines      155397   116000   -39397     
  Branches     2436     2438       +2     
==========================================
- Hits        69487    43993   -25494     
+ Misses      85673    71768   -13905     
- Partials      237      239       +2

Flag	Coverage Δ
harness	`?`
web	`35.07% <0.00%> (-0.15%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
...src/e2e/models/components/AddUsersToGroupsModal.ts	`0.00% <0.00%> (ø)`
...act/src/e2e/models/components/SetUserRolesModal.ts	`0.00% <0.00%> (ø)`
...react/src/e2e/models/components/SkeletonSection.ts	`0.00% <0.00%> (ø)`
...t/src/e2e/models/components/Table/SkeletonTable.ts	`0.00% <0.00%> (ø)`
.../src/e2e/models/components/WorkspaceDeleteModal.ts	`0.00% <0.00%> (ø)`
.../react/src/e2e/models/components/WorkspacesList.ts	`0.00% <0.00%> (ø)`
webui/react/src/e2e/models/hew/Dropdown.ts	`0.00% <0.00%> (ø)`
webui/react/src/e2e/models/hew/SplitPane.ts	`0.00% <0.00%> (ø)`
webui/react/src/e2e/models/ant/Switch.ts	`0.00% <0.00%> (ø)`
...ebui/react/src/e2e/models/components/Navigation.ts	`0.00% <0.00%> (ø)`
... and 21 more

... and 324 files with indirect coverage changes

djanicekpach

overall I like it but sharing users in a session map is probably going to cause race conditions so I think we should still try to have each test only interact with users it creates.

If there is something really tricky like pagination that requires 10+ users we could hold off and see if we can use mocks for that. It would be faster and impervious to environment shenanigans. We can have a mock return 1000 users if we want and we don't have to create or manage those users.

djanicekpach · 2024-04-23T19:18:52Z

webui/react/src/e2e/utils/naming.ts

+  return {
+    safeName: (baseName: string): string => {
+      const timestamp = Date.now();
+      return `${baseName}${sessionRandomHash}${randIdAlphanumeric()}_${timestamp}`;


Could we just use a simpler UUID for to avoid collision? Generating random ids is not the fastest operation, and it seems a little weird to do two calls to Math.random() and get a timestamp when a single uuid.v4() is virtually guaranteed not to collide.

I'm also not a fan of putting meaningful information in unique IDs. It always makes it harder to change down the line. If we need to know what tests made which users when, we should have a debug log for that rather than putting it in an ID.

meaningful information in unique IDs

that's a good point. i always think it's best to name a test case rather than generate it's name from the types of parameters it has, for example.

Could we just use a simpler UUID

the session random hash is specifically useful in situations like this one. using the session hash, I can find all the users i made "during this test session" with

await userManagementPage.search.pwLocator.fill(usernamePrefix + sessionRandomHash);

rather than all the users that just match the prefix, which would collide.

if i had 10 unique ID users, there's no way I could gather them all with a single search unless I used a prefix, but that prefix would be guaranteed to fail on a dirty environment. After the hashes, I figured it would be useful to anyone using a dirty environment to see a timestamp. I agree that we shouldn't be relying on the timestamp for collision or test meaning.

if it's alright with you, I'll throw this explanation into a docstring or readme-- I really need to keep the session hash. I don't know another way around it. Also, it's not 2n calls to Math.random(), it's n+1.

webui/react/src/e2e/fixtures/user.fixture.ts

djanicekpach · 2024-04-23T19:45:13Z

webui/react/src/e2e/fixtures/user.fixture.ts

@@ -16,9 +16,11 @@ interface UserArgs {
  isAdmin?: boolean;
 }

+// one list of users per test session
+const users = new Map<string, User>();


This maybe a race condition if it is truly shared across tests. Setting a map is likely not an atomic operation It has to do a Get on the map, then save the map with a new value to the place in memory. If two tests do this at the same time it could go like:

test1 gets the map

test2 gets the map

test1 set's it's user and persists.

test2 sets it's user and persists, overriding the new user from test1 since the initial GET occurred before the user was added

I think trying to have a list of all users across all tests is not a great practice anyway since if we want to shard the runners in the future this becomes an incomplete list.

yeah this is a good point. ultimately, I only use this list to tear down uses. But I wouldn't want Suite A to teardown users from Suite B. let me think on how to fix this.

In the case of shared runners, I believe they would each still load their own modules, and so they wouldn't collide. But if one runner was executing multiple test suites at once via threading, then we'd be in trouble.

djanicekpach · 2024-04-23T19:54:34Z

webui/react/src/e2e/fixtures/user.fixture.ts

+    await expect(row.status.pwLocator).toContainText(isActive ? 'Active' : 'Inactive');
+  }
+
+  async deactivateTestUsersOnTable(): Promise<void> {


This seems scary to try to pull all users and deactivate them. It could blow up other tests on shared test environments if we try to wipe them from all tests. Not that many tests will create users right? Can each test that creates users track their users and delete them rather than trying to archive all of them?

(I assumed this used the username session hash from safeUsername() to only remove this test sessions users, but I don't see that in allRowkeys or anywhere else, so now I'm not sure)

yeah this is a helper method that clicks this button in blue.

I agree it's pretty scary, in the test implementation, I actually check that all the users on the table were made in this session. Maybe i can put something similar into the method itself

await expect(async () => { expect(await userManagementPage.table.table.filterRows(async (row) => { return (await row.user.name.pwLocator.textContent())?.indexOf(usernamePrefix) === 0; })).toHaveLength(10); }).toPass({timeout: 10000});

if we are checking that the test created the user, should we just start with the list of users and only delete those?

should we just start with the list of users and only delete those

yeah that's what i'm doing with this.

// set pagination to 10 await userManagementPage.table.table.pagination.perPage.pwLocator.click(); await userManagementPage.table.table.pagination.perPage.perPage10.pwLocator.click(); // filter by active users await userManagementPage.filterStatus.pwLocator.click(); await userManagementPage.filterStatus.activeUsers.pwLocator.click(); // search for users created this session and wait for table stable await userManagementPage.search.pwLocator.fill(usernamePrefix + sessionRandomHash);

on a clean environment, i could get away with this

await userManagementPage.search.pwLocator.fill(usernamePrefix);

locally, my environment will never be clean, so i include the session hash

djanicekpach · 2024-04-23T19:58:35Z

webui/react/src/e2e/models/hew/Nameplate.ts

+ * @param {string} obj.selector - Used instead of `defaultSelector`
+ */
+export class Nameplate extends NamedComponent {
+  override defaultSelector: string = '[class^="Nameplate_base"]'; // must be provided


// must be provided

can we remove this comment? I don't think it adds anything when the selector is non-blank

djanicekpach · 2024-04-23T20:02:50Z

webui/react/src/e2e/models/hew/Nameplate.ts

+export class Nameplate extends NamedComponent {
+  override defaultSelector: string = '[class^="Nameplate_base"]'; // must be provided
+
+  #nameSelector = '.ant-typography:last-of-type';


last-of-type seems like a fragile way to define a selector. Do we have a way forward to use another selector? (potentially in a future change if it's involved)

our way forward would be with a change to hew. i felt clever coming up with this solution though!

djanicekpach · 2024-04-23T20:18:17Z

webui/react/src/e2e/tests/userManagement.spec.ts

-        await expect(async () => {
-          await userFixtureSetupTeardown.deactivateTestUsers();
-        }).toPass({ timeout: 20_000 });
+        await userFixtureSetupTeardown.deactivateAllTestUsers();


Doesn't this cause problems for other describe blocks if they make a user during the test?

I don't think it's an issue for parallel. And it can't be an issue for single-worker.
https://playwright.dev/docs/test-parallel#parallelize-tests-in-a-single-file

Note that parallel tests are executed in separate worker processes and cannot share any state or global variables. Each test executes all relevant hooks just for itself, including beforeAll and afterAll.

doesn't user fixture pull the users from the table though with this? const ids = await this.userManagementPage.table.table.allRowKeys(); and then on line #145 we set the user to the map.

okay so two things. the part you mentioned is deactivateTestUsersOnTable. table.allRowKeys(); in that function is used to manage the internal state. "physically", the test is using this to set the status in the product.

await this.userManagementPage.table.table.headRow.selectAll.pwLocator.click();

however, this line here is referencing deactivateAllTestUsers. That method will only go through the users from this test session. It manually searches for each user one at a time, so there shouldn't be a risk of search collision.

for await (const user of users.values()) {

JComins000 requested a review from a team as a code owner April 22, 2024 16:00

cla-bot bot added the cla-signed label Apr 22, 2024

JComins000 changed the base branch from jcom/INFENG-455/user-tests to main April 22, 2024 16:06

JComins000 requested review from a team as code owners April 22, 2024 16:06

JComins000 requested review from EmilyBonar and loksonarius April 22, 2024 16:06

djanicekpach self-requested a review April 22, 2024 16:13

JComins000 force-pushed the jcom/INFENG-455/user-tests-continued branch from d64a121 to 1c10053 Compare April 22, 2024 18:13

JComins000 commented Apr 22, 2024

View reviewed changes

djanicekpach requested changes Apr 23, 2024

View reviewed changes

JComins000 force-pushed the jcom/INFENG-455/user-tests-continued branch 3 times, most recently from 08b4a49 to f4f5188 Compare April 24, 2024 20:24

JComins000 added 6 commits April 24, 2024 13:32

one commit

e51f5b2

fix auth spec

48a2a52

more merge conflicts

7e7cc4a

job names

834bd3b

fixing error on auth spec

62a9bda

maybe this will help

64802f1

JComins000 force-pushed the jcom/INFENG-455/user-tests-continued branch 2 times, most recently from 78efd7c to bdaf43b Compare April 24, 2024 22:02

maybe this will help

bf50b15

JComins000 force-pushed the jcom/INFENG-455/user-tests-continued branch from bdaf43b to bf50b15 Compare April 24, 2024 23:17

JComins000 added 3 commits April 25, 2024 14:48

redoing login logic for the better

d9c21b2

this is such a pain

c7d84bb

let's see if disabling parrallel can help

ed665bb

JComins000 force-pushed the jcom/INFENG-455/user-tests-continued branch from a18cf89 to ed665bb Compare April 25, 2024 23:48

JComins000 added 2 commits April 26, 2024 10:33

another small fix

eeadace

Merge branch 'main' into jcom/INFENG-455/user-tests-continued

ee2861c

djanicekpach approved these changes Apr 26, 2024

View reviewed changes

JComins000 enabled auto-merge (squash) April 26, 2024 17:50

JComins000 disabled auto-merge April 26, 2024 17:51

JComins000 added 2 commits April 26, 2024 10:54

auth test deflake

6558270

validate user stability

a473f14

JComins000 merged commit bd7b5ef into main Apr 26, 2024
87 of 99 checks passed

JComins000 deleted the jcom/INFENG-455/user-tests-continued branch April 26, 2024 19:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: user tests continued [INFENG-455] #9214

test: user tests continued [INFENG-455] #9214

JComins000 commented Apr 22, 2024 •

edited

Loading

netlify bot commented Apr 22, 2024 •

edited

Loading

JComins000 Apr 22, 2024

JComins000 Apr 22, 2024

codecov bot commented Apr 22, 2024 •

edited

Loading

djanicekpach left a comment

djanicekpach Apr 23, 2024

JComins000 Apr 23, 2024 •

edited

Loading

djanicekpach Apr 23, 2024

JComins000 Apr 23, 2024

JComins000 Apr 23, 2024

djanicekpach Apr 23, 2024

JComins000 Apr 23, 2024

djanicekpach Apr 23, 2024

JComins000 Apr 23, 2024 •

edited

Loading

djanicekpach Apr 23, 2024

djanicekpach Apr 23, 2024

JComins000 Apr 23, 2024

djanicekpach Apr 23, 2024

JComins000 Apr 23, 2024 •

edited

Loading

djanicekpach Apr 23, 2024

JComins000 Apr 23, 2024

test: user tests continued [INFENG-455] #9214

test: user tests continued [INFENG-455] #9214

Conversation

JComins000 commented Apr 22, 2024 • edited Loading

Ticket

Description

Test Plan

Checklist

netlify bot commented Apr 22, 2024 • edited Loading

✅ Deploy Preview for determined-ui ready!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Apr 22, 2024 • edited Loading

Codecov Report

djanicekpach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JComins000 Apr 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JComins000 Apr 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JComins000 Apr 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JComins000 commented Apr 22, 2024 •

edited

Loading

netlify bot commented Apr 22, 2024 •

edited

Loading

codecov bot commented Apr 22, 2024 •

edited

Loading

JComins000 Apr 23, 2024 •

edited

Loading

JComins000 Apr 23, 2024 •

edited

Loading

JComins000 Apr 23, 2024 •

edited

Loading