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: experiment list sort [INFENG-766] #9675

Merged
merged 6 commits into from
Jul 22, 2024
Merged

Conversation

JComins000
Copy link
Contributor

@JComins000 JComins000 commented Jul 17, 2024

Ticket

INFENG-766

Description

This is the example I gave during my dev presentation. It's good to have this on main because we'll be closer to running correctly against shared environments.

Test Plan

Checklist

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

@JComins000 JComins000 requested review from a team as code owners July 17, 2024 20:30
@JComins000 JComins000 requested a review from ashtonG July 17, 2024 20:30
@cla-bot cla-bot bot added the cla-signed label Jul 17, 2024
Copy link

netlify bot commented Jul 17, 2024

Deploy Preview for determined-ui ready!

Name Link
🔨 Latest commit 23f4ada
🔍 Latest deploy log https://app.netlify.com/sites/determined-ui/deploys/66999cfb0044a90008a5d4c3
😎 Deploy Preview https://deploy-preview-9675--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 Jul 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 48.70%. Comparing base (274d763) to head (23f4ada).
Report is 10 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9675      +/-   ##
==========================================
- Coverage   53.32%   48.70%   -4.63%     
==========================================
  Files        1254      931     -323     
  Lines      152655   123432   -29223     
  Branches     3244     3243       -1     
==========================================
- Hits        81398    60113   -21285     
+ Misses      71105    63167    -7938     
  Partials      152      152              
Flag Coverage Δ
harness ?
web 51.55% <100.00%> (+<0.01%) ⬆️

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

Files Coverage Δ
webui/react/src/components/MultiSortMenu.tsx 88.51% <100.00%> (+0.27%) ⬆️

... and 323 files with indirect coverage changes

Comment on lines 89 to 90
// [ET-284] wait before popover close or else it'll flake
await authedPage.waitForTimeout(1_000);
Copy link
Contributor

Choose a reason for hiding this comment

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

i'm wondering if there's a better way to wait for the page to settle other than adding a timeout. Menu state is debounced to keep the UI responsive for users -- while we're planning a fix to ensure the menu state is committed on close, waiting for a matching request should decrease the amount of time that the test is waiting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

since the fix is planned, i think i'd rather just wait for the debounce to end for now. it'll be easy to delete the line later once the work gets done. i've got other stuff i need to focus on for now. thanks for approving

Copy link
Contributor

Choose a reason for hiding this comment

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

i mean, it would also work for the current waitTableStable stuff as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i just added it to my backlog. thanks for the suggestion

@JComins000 JComins000 force-pushed the jcom/noissue/exptracking-sort branch from 722437c to 1019417 Compare July 18, 2024 21:16
@JComins000 JComins000 merged commit f721751 into main Jul 22, 2024
81 of 94 checks passed
@JComins000 JComins000 deleted the jcom/noissue/exptracking-sort branch July 22, 2024 20:03
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.

3 participants