test: user-page-models #9084

JComins000 · 2024-04-01T23:24:09Z

Description

This should be most of the page model changes that come with the user tests. This includes the nav sidebar and interactive tables, and all the other pieces of the user admin page.

There's also a test pattern here where we begin test setup with creating a user, and teardown by deactivating it, all through the UI. Unfortunately, I couldn't find a way to delete test users. We would want to consider running teardown at test setup time as well, or maybe a separate reaper script. Neither are happening at this moment, so this would be a good place to begin that discussion.

Test Plan

Commentary (optional)

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.

Ticket

INFENG-455

netlify · 2024-04-01T23:24:26Z

✅ Deploy Preview for determined-ui ready!

Name	Link
🔨 Latest commit	`89c1f52`
🔍 Latest deploy log	https://app.netlify.com/sites/determined-ui/deploys/660f087b62f777000886434c
😎 Deploy Preview	https://deploy-preview-9084--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.

codecov · 2024-04-01T23:29:46Z

Codecov Report

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

Project coverage is 40.30%. Comparing base (f78b9aa) to head (89c1f52).
Report is 12 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9084      +/-   ##
==========================================
- Coverage   47.07%   40.30%   -6.78%     
==========================================
  Files        1155      848     -307     
  Lines      142400   103887   -38513     
  Branches     2421     2444      +23     
==========================================
- Hits        67034    41869   -25165     
+ Misses      75176    61813   -13363     
- Partials      190      205      +15

Flag	Coverage Δ
harness	`?`
web	`38.51% <5.81%> (-0.44%)`	⬇️

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

Files	Coverage Δ
...bui/react/src/components/AddUsersToGroupsModal.tsx	`78.48% <100.00%> (ø)`
...bui/react/src/components/ChangeUserStatusModal.tsx	`84.21% <100.00%> (ø)`
webui/react/src/components/CreateUserModal.tsx	`90.29% <100.00%> (+0.74%)`	⬆️
webui/react/src/components/SetUserRolesModal.tsx	`73.03% <100.00%> (+0.30%)`	⬆️
webui/react/src/components/SkeletonSection.tsx	`82.35% <100.00%> (ø)`
...ui/react/src/components/Table/InteractiveTable.tsx	`70.76% <100.00%> (+0.17%)`	⬆️
webui/react/src/components/Table/SkeletonTable.tsx	`97.22% <100.00%> (ø)`
webui/react/src/pages/Admin/UserManagement.tsx	`83.39% <100.00%> (+0.11%)`	⬆️
webui/react/src/components/Navigation.tsx	`0.00% <0.00%> (ø)`
webui/react/src/e2e/models/pages/SignIn.ts	`0.00% <0.00%> (ø)`
... and 22 more

... and 326 files with indirect coverage changes

djanicekpach

Looks good, mostly style comments/questions

djanicekpach · 2024-04-02T14:15:27Z

webui/react/src/components/CreateUserModal.tsx

+              data-testid="username"
+              disabled={!!user}
+              maxLength={128}
+              placeholder="Username"


Place holder changed here as well. Was that intentional?

yeah the space was bugging me. It was intentional if reviewers are okay with it

let's not introduce copy changes in the testing pr.

just fyi, "username" is used elsewhere as well. I'll make another PR for the fix if you're okay with it

djanicekpach · 2024-04-02T15:52:24Z

webui/react/src/e2e/models/BaseComponent.ts

+   */
+  get root(): BasePage {
+    let root: parentTypes = this._parent;
+    for (; !(root instanceof BasePage); root = root._parent) {


I think I prefer while here. Just feels better to me than having 2 empty fields

Suggested change

for (; !(root instanceof BasePage); root = root._parent) {

while(!(root instanceof BasePage)){

root = root._parent

}

djanicekpach · 2024-04-02T15:55:00Z

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

+  debugComponentVisible(component: BaseComponent): void {
+    const componentTree: parentTypes[] = [];
+    let root: parentTypes = component;
+    for (; !(root instanceof BasePage); root = root._parent) {


Same style comment as the other for vs. while line. If you really want to get fancy you could make an iterator function for this that runs the same operation on each element, but probably overkill.

djanicekpach · 2024-04-02T15:56:19Z

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

+  debugComponentVisible(component: BaseComponent): void {
+    const componentTree: parentTypes[] = [];
+    let root: parentTypes = component;
+    for (; !(root instanceof BasePage); root = root._parent) {


the underscore in _parent implies private/protected to me, so to access it outside the inheritance hierarchy, I think we either should have a getter for this field, or remove the underscore and allow it to be public. I think I prefer the getter option.

why do you prefer the getter? it's already readonly, so i don't think we need to hide the attribute. i'd rather remove the underscore

djanicekpach · 2024-04-02T16:20:28Z

webui/react/src/e2e/models/components/AddUsersToGroupsModal.ts

+ * @param {string} [obj.selector] - Used instead of `defaultSelector`
+ */
+export class AddUsersToGroupsModal extends Modal {
+  static override defaultSelector: string = Modal.defaultSelector;


do we actually have to override this if we are just using the the parent selector?

yes. another reason to switch to instance property

djanicekpach · 2024-04-02T16:59:02Z

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

+      await userManagementPage.createUserModal.username.pwLocator.fill(username);
+      await userManagementPage.createUserModal.footer.submit.pwLocator.click();
+      const newIDs = await userManagementPage.table.table.newRowKeys(oldIDs);
+      expect(newIDs, `newids ${newIDs}, oldids ${oldIDs}`).toHaveLength(1);


Could we get the ID from the username? If the username has a uuid do we even need the internal ID for the test? We could just find the row with the username.

djanicekpach · 2024-04-02T17:06:54Z

webui/react/src/e2e/models/README.md

-  readonly username: BaseComponent = new BaseComponent({ parent: this.form, selector: "input[data-testid='username']" });
-  readonly password: BaseComponent = new BaseComponent({ parent: this.form, selector: "input[data-testid='password']" });
-  readonly submit: BaseComponent = new BaseComponent({ parent: this.form, selector: "button[data-testid='submit']" });
+  readonly #form: BaseComponent = new BaseComponent({ parent: this, selector: `form` });


For my own edification, why did this switch to having # starting the variable?

# is for private.
I was thinking it's useful for the hierarchy, but we never .click() or .fill() it's pwlocator.

webui/react/src/components/Table/InteractiveTable.tsx

webui/react/src/e2e/models/components/Table/InteractiveTable.ts

ashtonG · 2024-04-03T17:15:26Z

webui/react/src/e2e/models/components/Table/InteractiveTable.ts

+     * @param {string} value - value of the row attribute
+     */
+    return (value: string) => {
+      // TODO default selector should be instance property to make this easier. We want RowType.defaultSelector


could you talk a little about how making the default selector an instance property would make this easier?

yeah, here's the change
#9096

the problem is that we want to use selector: RowType.defaultSelector + [${key}="${value}"],, but TS doesn't know about any static elements on the generic class RowType. I put Row.defaultSelector here to make the compiler happy, and it works for now.

class UserRow extends Row { static override defaultSelector: string = Row.defaultSelector;

With the instance property, the lines here change to

return new this.#rowType({ attachment: `[${key}="${value}"]`,

that's fixable -- the issue you're running into is that the type of a class is its instance type in typescript -- when referring to the class itself, you want the typeof the class:

class Coordinates { static memo = "hello" as const x = 0 y = 0 } const c: Coordinates = new Coordinates(); const s: (typeof Coordinates)['memo'] = "hello"

yeah I didn't like using the [] syntax to access object properties. It was also telling me a few errors when I had tried

the error message is saying that you're using the type where you want to use a value -- shouldn't it be this.rowType.defaultSelector?

yes, but TS doesn't actually know that the static value exists

and here's the implementation from basecomponent.ts

export abstract class NamedComponent extends BaseComponent { constructor({ parent, selector }: { parent: parentTypes; selector: string }) { super({ parent, selector }); requireStaticArgs(this.constructor, ['defaultSelector']); } } // TODO remove this for instance properties instead. static has been difficult to work with export function requireStaticArgs<T extends object>(obj: T, requiredProperties: string[]): void { requiredProperties.forEach((requiredProp) => { if (!Object.hasOwn(obj, requiredProp)) { throw new Error(`${obj} must declare a static ${requiredProp}!`); } }); }

that's because of the RowTypeGeneric -- you don't need that with the typeof because it knows the type of the constructor already and changing the type of the constructor would break the extension

Co-authored-by: Ashton G. <ashton.galloway@hpe.com>

djanicekpach · 2024-04-03T18:51:21Z

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

+    await expect(page).toHaveURL(userManagementPage.url);
+  });
+
+  // test('Users table count matches admin page users tab', async () => {


delete comment before merge

JComins000 added 19 commits March 26, 2024 15:11

added nav sidebar and menu items

f9346b5

unflatten menu items

92506dd

better organized

2bcfb59

navigation test

6b98385

page models except for the table and action bar

eb0e2e7

base admin page

82be96e

skeleton table in place, now just missing the table itself

640d11f

before a big find replace

6d03c00

on the outside of selectors

ad51f9b

ready to start working on hew/table tomorrow

db20f5e

forgot to git add untracked files yesterday

33fa73f

working on getRows next

1a89164

formatting

2815a6a

looking for a way to set row data-testid and cell data-testid

e9067c4

header row done

5179441

getByRow exists

4b41421

user page models.. complete?? lets see if they break

9431eda

amazing progress

6085b4e

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

df8053a

JComins000 requested a review from a team as a code owner April 1, 2024 23:24

JComins000 requested a review from ashtonG April 1, 2024 23:24

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

doc changes

02fc265

JComins000 force-pushed the jcom/INFENG-455/user-tests branch 2 times, most recently from 2b40ecf to 1847fbb Compare April 2, 2024 15:25

tsconfig shouldnt change

cddd7a3

JComins000 force-pushed the jcom/INFENG-455/user-tests branch from 1847fbb to cddd7a3 Compare April 2, 2024 15:26

djanicekpach requested changes Apr 2, 2024

View reviewed changes

JComins000 added 7 commits April 2, 2024 10:46

row filtering code written during standup

8ee337c

PR feedback, testhooks for cells

befe1ec

missing await

d21f83a

retry logic

7366a0e

wait for one row

f53c138

comments and remove the placeholder change

6bc2c80

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

791c78c

ashtonG reviewed Apr 3, 2024

View reviewed changes

webui/react/src/components/Table/InteractiveTable.tsx Show resolved Hide resolved

ashtonG reviewed Apr 3, 2024

View reviewed changes

JComins000 and others added 2 commits April 3, 2024 12:06

Update webui/react/src/e2e/models/components/Table/InteractiveTable.ts

774d762

Co-authored-by: Ashton G. <ashton.galloway@hpe.com>

Update webui/react/src/e2e/models/components/Table/InteractiveTable.ts

c96095e

Co-authored-by: Ashton G. <ashton.galloway@hpe.com>

djanicekpach reviewed Apr 3, 2024

View reviewed changes

JComins000 added 3 commits April 3, 2024 15:53

no comments

37f9f2e

fixing some typos in Ashton's suggestions

b732b5b

formatting

869192b

djanicekpach approved these changes Apr 4, 2024

View reviewed changes

ashtonG approved these changes Apr 4, 2024

View reviewed changes

test: Jcom/infeng 455/default selector instance prop (#9096)

89c1f52

JComins000 enabled auto-merge (squash) April 4, 2024 20:08

JComins000 merged commit 2c6fec7 into main Apr 4, 2024
66 of 80 checks passed

JComins000 deleted the jcom/INFENG-455/user-tests branch April 4, 2024 20:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: user-page-models #9084

test: user-page-models #9084

JComins000 commented Apr 1, 2024 •

edited by jira bot

Loading

netlify bot commented Apr 1, 2024 •

edited

Loading

codecov bot commented Apr 1, 2024 •

edited

Loading

djanicekpach left a comment

djanicekpach Apr 2, 2024

JComins000 Apr 2, 2024

ashtonG Apr 2, 2024

JComins000 Apr 2, 2024

djanicekpach Apr 2, 2024

djanicekpach Apr 2, 2024

djanicekpach Apr 2, 2024

JComins000 Apr 2, 2024

djanicekpach Apr 2, 2024

JComins000 Apr 2, 2024

djanicekpach Apr 2, 2024

djanicekpach Apr 2, 2024

JComins000 Apr 2, 2024 •

edited

Loading

ashtonG Apr 3, 2024

JComins000 Apr 3, 2024

ashtonG Apr 3, 2024

JComins000 Apr 3, 2024

ashtonG Apr 4, 2024

JComins000 Apr 4, 2024

JComins000 Apr 4, 2024

ashtonG Apr 4, 2024

djanicekpach Apr 3, 2024

test: user-page-models #9084

test: user-page-models #9084

Conversation

JComins000 commented Apr 1, 2024 • edited by jira bot Loading

Description

Test Plan

Commentary (optional)

Checklist

Ticket

netlify bot commented Apr 1, 2024 • edited Loading

✅ Deploy Preview for determined-ui ready!

codecov bot commented Apr 1, 2024 • edited Loading

Codecov Report

djanicekpach left a 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

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 2, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JComins000 commented Apr 1, 2024 •

edited by jira bot

Loading

netlify bot commented Apr 1, 2024 •

edited

Loading

codecov bot commented Apr 1, 2024 •

edited

Loading

JComins000 Apr 2, 2024 •

edited

Loading