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

feat: add e2e tests for multi-sort filter on experiments lista #9992

Open
wants to merge 27 commits into
base: main
Choose a base branch
from

Conversation

thiagodallacqua-hpe
Copy link
Contributor

@thiagodallacqua-hpe thiagodallacqua-hpe commented Sep 25, 2024

Ticket

ET-752

Description

This PR intends to add e2e tests for the multi-sort filter for the experiment list.

Test Plan

run the test 'Multi-sort menu' test scripts on playwrite.

Screenshot 2024-09-25 at 16 30 23

Screenshot 2024-09-25 at 16 34 22

Screenshot 2024-10-03 at 17 39 32

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

Copy link

netlify bot commented Sep 25, 2024

Deploy Preview for determined-ui ready!

Name Link
🔨 Latest commit 49f1ab8
🔍 Latest deploy log https://app.netlify.com/sites/determined-ui/deploys/670573b4019db70007b68d17
😎 Deploy Preview https://deploy-preview-9992--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.

@thiagodallacqua-hpe thiagodallacqua-hpe changed the title thiago/752 feat: add e2e tests for multi-sort filter on experiments lista Sep 25, 2024
Copy link

codecov bot commented Sep 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 50.56%. Comparing base (9faa0cb) to head (49f1ab8).

❗ There is a different number of reports uploaded between BASE (9faa0cb) and HEAD (49f1ab8). Click for more details.

HEAD has 8 uploads less than BASE
Flag BASE (9faa0cb) HEAD (49f1ab8)
harness 8 0
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9992      +/-   ##
==========================================
- Coverage   59.17%   50.56%   -8.62%     
==========================================
  Files         755      949     +194     
  Lines      104932   128621   +23689     
  Branches     3615     3616       +1     
==========================================
+ Hits        62098    65039    +2941     
- Misses      42701    63449   +20748     
  Partials      133      133              
Flag Coverage Δ
harness ?
web 54.34% <ø> (ø)

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

see 814 files with indirect coverage changes

@thiagodallacqua-hpe thiagodallacqua-hpe requested review from JComins000 and a team and removed request for gt2345 September 25, 2024 19:31
Copy link
Contributor

@ashtonG ashtonG left a comment

Choose a reason for hiding this comment

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

This test isn't testing the sort order. The sort settings aren't used in the cli call that you're using. You can also tell that the test isn't properly testing by swapping the checkTableOrder calls.

Please test that the sort order is being applied and working in the frontend.

  1. ensure that you're picking columns that can be tested. Because we're using the same config for all the experiments in this project, sorting on trial count and searcher will not affect the sort order.
  2. check the status of the search in the dom. you can do this by checking that the column values are in the order that you expect them to be and/or ensuring that the call to experiments-search was made with the parameters you expect. Obviously given the context the former would be better than the latter.

Refactor the getColumnTextValue function in experimentList.spec.ts to simplify
the code and improve readability. Update function calls to reflect changes in
the function implementation.
Co-authored-by: Ashton G. <ashton.galloway@hpe.com>
@thiagodallacqua-hpe
Copy link
Contributor Author

after the last changes, my local are still passing, so, let's see how the CI env will handle 😅

Screenshot 2024-09-30 at 16 30 31

cc @ashtonG

@thiagodallacqua-hpe
Copy link
Contributor Author

the current state of local after the last changes:

Screenshot 2024-10-03 at 17 39 32

webui/react/src/e2e/tests/experimentList.spec.ts Outdated Show resolved Hide resolved
webui/react/src/e2e/tests/experimentList.spec.ts Outdated Show resolved Hide resolved
webui/react/src/e2e/tests/experimentList.spec.ts Outdated Show resolved Hide resolved
webui/react/src/e2e/tests/experimentList.spec.ts Outdated Show resolved Hide resolved
webui/react/src/e2e/tests/experimentList.spec.ts Outdated Show resolved Hide resolved
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