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

test: datagrid tests [INFENG-687] #9400

Merged
merged 18 commits into from
May 31, 2024
Merged

Conversation

JComins000
Copy link
Contributor

@JComins000 JComins000 commented May 21, 2024

Ticket

INFENG-687

Description

these are datagrid tests

  • comes with a few extra test ids
  • a change to eslint
  • unparallel tests. we'll parallel them again later
  • async log(s: string) on base page
  • i made a debug.ts util. i'm open to using other names
  • dropdown popover and select are beginning to diverge a bit. I have a ticket to address this
  • this filter test implementation is bothering me a bit, so i have some more work to do later

by the way, there is still one more set of tests to write. it's all the pieces of the action menu. I'm gonna try and get the devs to write these parts.

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.

@cla-bot cla-bot bot added the cla-signed label May 21, 2024
Copy link

netlify bot commented May 21, 2024

Deploy Preview for determined-ui ready!

Name Link
🔨 Latest commit 724137f
🔍 Latest deploy log https://app.netlify.com/sites/determined-ui/deploys/665a02150cce8c00087fc81e
😎 Deploy Preview https://deploy-preview-9400--determined-ui.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

Copy link

codecov bot commented May 21, 2024

Codecov Report

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

Project coverage is 43.43%. Comparing base (0599d0e) to head (724137f).
Report is 9 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9400      +/-   ##
==========================================
- Coverage   48.62%   43.43%   -5.20%     
==========================================
  Files        1233      909     -324     
  Lines      159016   119090   -39926     
  Branches     2777     2778       +1     
==========================================
- Hits        77322    51725   -25597     
+ Misses      81520    67191   -14329     
  Partials      174      174              
Flag Coverage Δ
harness ?
web 44.38% <0.00%> (-0.01%) ⬇️

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

Files Coverage Δ
...c/components/FilterForm/components/FilterField.tsx 0.00% <0.00%> (ø)
...ui/react/src/components/FilterForm/TableFilter.tsx 0.00% <0.00%> (ø)
webui/react/src/components/TableActionBar.tsx 0.00% <0.00%> (ø)

... and 324 files with indirect coverage changes

@JComins000 JComins000 force-pushed the jcom/INFENG-687/datagrid-tests branch from 0aa2a11 to 975bbb3 Compare May 22, 2024 23:26
@JComins000 JComins000 force-pushed the jcom/INFENG-687/datagrid-tests branch from b67bc12 to 99b7883 Compare May 24, 2024 00:39
@JComins000 JComins000 force-pushed the jcom/INFENG-687/datagrid-tests branch from 1b05a75 to f2011c5 Compare May 28, 2024 19:51
@JComins000 JComins000 force-pushed the jcom/INFENG-687/datagrid-tests branch from 4ff2b5a to 5a080f3 Compare May 29, 2024 18:48
@JComins000 JComins000 force-pushed the jcom/INFENG-687/datagrid-tests branch 6 times, most recently from 006203d to c9ce6bb Compare May 30, 2024 17:06
@JComins000 JComins000 force-pushed the jcom/INFENG-687/datagrid-tests branch 3 times, most recently from a564611 to caa39ab Compare May 30, 2024 18:22
@JComins000 JComins000 force-pushed the jcom/INFENG-687/datagrid-tests branch from caa39ab to b01efb7 Compare May 30, 2024 21:47
sourceType: 'module',
},
plugins: ['import', 'jsdoc', 'react', 'react-hooks', 'sort-keys-fix'],
root: true,
ignorePatterns: ['**/src/services/stream/wire.ts', '**/src/e2e/playwright-report/**'],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is a formatting change that came from precommit

webui/react/.eslintrc.js Outdated Show resolved Hide resolved
@@ -22,5 +22,5 @@
"jsx": "react-jsx",
"noFallthroughCasesInSwitch": true
},
"include": ["src/**/*", "typings/**/*.ts", "*.ts", "__mocks__/**/*"]
"include": ["src/**/*", "typings/**/*.ts", "*.ts", "__mocks__/**/*", ".eslintrc.js"]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

precommit was failing without this

@djanicekpach djanicekpach self-requested a review May 30, 2024 22:40
}
} while (await incrementScroll());
return false;
throw new Error(`Column with index ${index} not found.`);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this approach worked better than returning false

try {
return await this.scrollColumnIntoView(this.headRow.getColumnDef(s));
} catch (e) {
if ((e as Error).message.indexOf('Column with index') === 0) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure if there's a better way

Copy link
Contributor

Choose a reason for hiding this comment

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

in the future, given that you're throwing the error here, you could throw an error class that extends from Error

class IndexNotFoundError extends Error {}
class NameNotFoundError extends Error {}
// ...snip
throw new IndexNotFoundError(`Column with index ${index} not found.`);

that you then check your caught error is an instance of

} catch(e) {
  if(e instanceof IndexNotFoundError) {
    throw new NameNotFoundError(`Column with name ${s} not found.`)
  }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will do! I have a followup coming soon

webui/react/.eslintrc.js Show resolved Hide resolved
webui/react/.eslintrc.js Outdated Show resolved Hide resolved
Copy link
Contributor

@djanicekpach djanicekpach left a comment

Choose a reason for hiding this comment

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

LGTM, some minor comments. I also can't really evaluate the eslint changes, so someone from the web team should check those out.

webui/react/playwright.config.ts Show resolved Hide resolved
webui/react/src/e2e/models/hew/DataGrid.ts Show resolved Hide resolved
webui/react/src/e2e/tests/userManagement.spec.ts Outdated Show resolved Hide resolved
@JComins000 JComins000 marked this pull request as ready for review May 31, 2024 17:24
@JComins000 JComins000 requested review from a team as code owners May 31, 2024 17:24
@JComins000 JComins000 merged commit c49eeea into main May 31, 2024
84 of 97 checks passed
@JComins000 JComins000 deleted the jcom/INFENG-687/datagrid-tests branch May 31, 2024 17:44
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.

4 participants