test: Jcom/infeng 454/sign in tests #9013

JComins000 · 2024-03-16T00:11:23Z

Description

adds page models for sign in tests
add a new test for when we sign in with wrong creds
add test hooks into a few react components

Test Plan

the tests run on CI

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-454

netlify · 2024-03-16T00:11:41Z

✅ Deploy Preview for determined-ui ready!

Name	Link
🔨 Latest commit	`cf71f65`
🔍 Latest deploy log	https://app.netlify.com/sites/determined-ui/deploys/6601f09d9d3a6f00082727ee
😎 Deploy Preview	https://deploy-preview-9013--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.

djanicekpach · 2024-03-20T15:01:03Z

webui/react/src/components/DeterminedAuth.tsx

@@ -87,6 +87,7 @@ const DeterminedAuth: React.FC<Props> = ({ canceler }: Props) => {

  const loginForm = (
    <Form
+      data-test="authForm"


spelling: data-testid I think you mean?

actually, i like top level components to have data-test. when the form gets implemented in SignIn.tsx, sign in could give it a data-testid, but it wouldn't need to unless it was instantiating more than one of this component. I'll write this up in a guide somewhere.

Why do you like top level components to be slightly different? To me it reads like an extra style thing for maintainers to remember and no implied meaning.
If you do want the top level to be different, can it be explicit? Something like data-top-level-testid or data-formid? It'd be good if the name was different enough to indicate that this difference is intentional, and ideally it could also describe why it was different.

maybe this?

data-test-component='my-component'

Seems diffferent enough form data-testid to avoid confusion. works for me!

webui/react/src/components/DeterminedAuth.tsx

djanicekpach · 2024-03-20T15:03:57Z

webui/react/src/components/DeterminedAuth.tsx

          placeholder="password"
          prefix={<Icon name="lock" size="small" title="Password" />}
        />
      </Form.Item>
      {isBadCredentials && (
-        <p className={[css.errorMessage, css.message].join(' ')}>Incorrect username or password.</p>
+        <p className={[css.errorMessage, css.message].join(' ')} data-testid="error">Incorrect username or password.</p>


nitpick: maybe bad-cred-error or something in case there is ever more than one on the page?

there actually are two alerts since there's almost always one in the bottom right. I'd like to have a list of errors. Maybe a BaseComponents plural class?

A list of errors would be nice. Feel free to punt to out of scope and make a follow up ticke here though since it sounds like that might be more involved. I don't love plural classes in languages with generic types. We can just do a Array and get most of the functionality for a lot less code.

actually, this isn't even the notification that shows in our test. im gonna delete this change

djanicekpach · 2024-03-20T15:04:58Z

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

+      throw new Error('username must be defined')
+    }
+    if (typeof process.env.PW_PASSWORD === "undefined") {
+      throw new Error('username must be defined')


copy&paste: password instead of username.

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

djanicekpach · 2024-03-20T15:23:53Z

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

+  get locateSelf(): Locator {
+    if (typeof this._locator === 'undefined') {
+      this._locator = this._parent.pwLocatorFunction(this.#selector);
+      Object.freeze(this._locator)


Why are we freezing the object here? It makes me a little nervous. Hopefully playwright doesn't keep any mutable data on Locator classes under the hood!

I wanted the attribute to only be instantiated once. it's kind of like saying

const myLocator = page.locator('my_selector')

I didn't want to create the "const" until we started using it. it behaves like a readonly that we're allowed to write to once.

Object.freeze is a bit different than const or readonly. It prevents reassignment, but also prevents changing the properties of the underlying object. If playwright ever alters a property under the hood (it's their object so that's pretty reasonable) it could cause a crash.

I love having constants instead of variables, but for a protected variable with a public getter, I think the benefits of this lockdown are kind of minimal when compared to the potential consequences. If you really want to protect it you could make a setter and either declare it private, or throw an error when it gets set and already has a value.

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

djanicekpach · 2024-03-20T15:26:10Z

webui/react/src/e2e/models/pages/SignIn.ts

+export class SignIn extends BasePage {
+  readonly pageComponent: PageComponent = new PageComponent({parent: this});
+  readonly detAuth: DeterminedAuth = new DeterminedAuth({parent: this.pageComponent})
+  // TODO add SSO page model as well


This seems probably out of scope, so feel free to make a ticket for sso. I think a lot more setup has to go into it.

djanicekpach · 2024-03-22T15:48:28Z

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

+  get locateSelf(): Locator {
+    if (typeof this._locator === 'undefined') {
+      this._locator = this._parent.pwLocatorFunction(this.#selector);
+      Object.freeze(this._locator)


Object.freeze is a bit different than const or readonly. It prevents reassignment, but also prevents changing the properties of the underlying object. If playwright ever alters a property under the hood (it's their object so that's pretty reasonable) it could cause a crash.

I love having constants instead of variables, but for a protected variable with a public getter, I think the benefits of this lockdown are kind of minimal when compared to the potential consequences. If you really want to protect it you could make a setter and either declare it private, or throw an error when it gets set and already has a value.

djanicekpach · 2024-03-22T16:05:24Z

webui/react/src/components/DeterminedAuth.tsx

@@ -87,6 +87,7 @@ const DeterminedAuth: React.FC<Props> = ({ canceler }: Props) => {

  const loginForm = (
    <Form
+      data-test="authForm"


Why do you like top level components to be slightly different? To me it reads like an extra style thing for maintainers to remember and no implied meaning.
If you do want the top level to be different, can it be explicit? Something like data-top-level-testid or data-formid? It'd be good if the name was different enough to indicate that this difference is intentional, and ideally it could also describe why it was different.

djanicekpach · 2024-03-22T16:08:09Z

webui/react/src/components/DeterminedAuth.tsx

          placeholder="password"
          prefix={<Icon name="lock" size="small" title="Password" />}
        />
      </Form.Item>
      {isBadCredentials && (
-        <p className={[css.errorMessage, css.message].join(' ')}>Incorrect username or password.</p>
+        <p className={[css.errorMessage, css.message].join(' ')} data-testid="error">Incorrect username or password.</p>


A list of errors would be nice. Feel free to punt to out of scope and make a follow up ticke here though since it sounds like that might be more involved. I don't love plural classes in languages with generic types. We can just do a Array and get most of the functionality for a lot less code.

djanicekpach · 2024-03-25T16:26:18Z

webui/react/playwright.config.ts

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

  /* https://playwright.dev/docs/test-timeouts#global-timeout */
-  globalTimeout: 3 * 60 * 1000, // 3 min
+  globalTimeout: process.env.PWDEBUG ? 0 : 3 * 60 * 1000, // 3 min unless debugging


Nice! I was thinking about doing this and forgot.

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

codecov · 2024-03-25T21:49:58Z

Codecov Report

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

Project coverage is 41.63%. Comparing base (e4bc377) to head (cf71f65).
Report is 3 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9013      +/-   ##
==========================================
- Coverage   47.80%   41.63%   -6.18%     
==========================================
  Files        1161      847     -314     
  Lines      143646   104428   -39218     
  Branches     2373     2377       +4     
==========================================
- Hits        68673    43475   -25198     
+ Misses      74820    60794   -14026     
- Partials      153      159       +6

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

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

Files	Coverage Δ
webui/react/src/components/DeterminedAuth.tsx	`0.00% <0.00%> (ø)`
webui/react/src/e2e/models/components/Page.ts	`0.00% <0.00%> (ø)`
webui/react/src/e2e/models/pages/SignIn.ts	`0.00% <0.00%> (ø)`
webui/react/src/e2e/models/utils/error.ts	`0.00% <0.00%> (ø)`
webui/react/src/e2e/fixtures/auth.fixture.ts	`0.00% <0.00%> (ø)`
webui/react/src/e2e/models/BasePage.ts	`0.00% <0.00%> (ø)`
.../react/src/e2e/models/components/DeterminedAuth.ts	`0.00% <0.00%> (ø)`
webui/react/src/e2e/models/BaseComponent.ts	`0.00% <0.00%> (ø)`

... and 320 files with indirect coverage changes

djanicekpach and others added 6 commits March 14, 2024 08:32

local config

b2219e4

update readme with running instructions

b20be45

linter fixes

2848d8f

move tests to folder

51f34c5

page models are present, now i need to use them

9fb8596

here are the models

37d7362

cla-bot bot added the cla-signed label Mar 16, 2024

JComins000 added 16 commits March 17, 2024 19:51

this is good

f4870de

this is sick

3b58951

done for today

e66fe83

done for real

c5793cf

i finally got that friggin type right

413d445

formatting and non relative imports

01d560e

Merge branch 'main' into jcom/INFENG-454/sign-in-tests

0966f96

documentation

6681aa9

member names and attributes

d093948

doh

ad55b1f

doh

cdac4ff

todo in place

bebbe22

subcomponents

ddce075

loc.fill works

d53fb87

no need for haslocator, getslocator

23cd71c

it compiles but i dont like pwLocator

8a7f1b9

JComins000 force-pushed the jcom/INFENG-454/sign-in-tests branch from 54a0a8e to d1252b7 Compare March 19, 2024 23:23

locator stuff feels much better now

f0e6b67

JComins000 force-pushed the jcom/INFENG-454/sign-in-tests branch from d1252b7 to f0e6b67 Compare March 19, 2024 23:26

djanicekpach self-requested a review March 20, 2024 15:00

djanicekpach requested changes Mar 20, 2024

View reviewed changes

mandator default selectors

5a497f4

JComins000 added 6 commits March 21, 2024 10:42

ayoo got it all working

c71db11

more auth cahnges before i pull main back in

081ab90

Merge branch 'main' into jcom/INFENG-454/sign-in-tests

5d4dac2

just needs a text assertion

23e5bd1

missed one file

67dd40b

ready for review and merge

72ee1e1

djanicekpach approved these changes Mar 22, 2024

View reviewed changes

JComins000 added 2 commits March 22, 2024 12:13

let's merge this thing

3a5254d

readme

801e95d

JComins000 marked this pull request as ready for review March 22, 2024 19:26

JComins000 requested a review from a team as a code owner March 22, 2024 19:26

JComins000 requested a review from ashtonG March 22, 2024 19:26

JComins000 and others added 7 commits March 22, 2024 12:36

remove one test attribute

1cd02e4

obj.freeze is fine && locators can return multiple elements

98fce42

plural

aacfe9e

data-test -> data-test-component

d02bb12

no freeze

66ae0c7

use override instead of class creation

e49ca51

remove unused var

0d6a22f

djanicekpach reviewed Mar 25, 2024

View reviewed changes

login fixture redirect condition

b44c36c

ashtonG approved these changes Mar 25, 2024

View reviewed changes

webui/react/src/e2e/fixtures/auth.fixture.ts Outdated Show resolved Hide resolved

JComins000 added 5 commits March 25, 2024 13:10

linting

f5d4fbc

linting

dbbf6f1

expect import

bf532da

linting lol

67f3a4a

gotta run prettier too :rolling eyes emoji

cf71f65

JComins000 merged commit 60cb003 into main Mar 25, 2024
69 of 81 checks passed

JComins000 deleted the jcom/INFENG-454/sign-in-tests branch March 25, 2024 21:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: Jcom/infeng 454/sign in tests #9013

test: Jcom/infeng 454/sign in tests #9013

JComins000 commented Mar 16, 2024 •

edited

Loading

netlify bot commented Mar 16, 2024 •

edited

Loading

djanicekpach Mar 20, 2024

JComins000 Mar 22, 2024

djanicekpach Mar 22, 2024

JComins000 Mar 25, 2024

djanicekpach Mar 25, 2024

djanicekpach Mar 20, 2024

JComins000 Mar 22, 2024

djanicekpach Mar 22, 2024

JComins000 Mar 22, 2024

djanicekpach Mar 20, 2024

djanicekpach Mar 20, 2024

JComins000 Mar 22, 2024

djanicekpach Mar 22, 2024

djanicekpach Mar 20, 2024

djanicekpach Mar 22, 2024

djanicekpach Mar 22, 2024

djanicekpach Mar 22, 2024

djanicekpach Mar 25, 2024

codecov bot commented Mar 25, 2024 •

edited

Loading

test: Jcom/infeng 454/sign in tests #9013

test: Jcom/infeng 454/sign in tests #9013

Conversation

JComins000 commented Mar 16, 2024 • edited Loading

Description

Test Plan

Commentary (optional)

Checklist

Ticket

netlify bot commented Mar 16, 2024 • edited Loading

✅ Deploy Preview for determined-ui ready!

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

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

codecov bot commented Mar 25, 2024 • edited Loading

Codecov Report

JComins000 commented Mar 16, 2024 •

edited

Loading

netlify bot commented Mar 16, 2024 •

edited

Loading

codecov bot commented Mar 25, 2024 •

edited

Loading