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: project create and delete react e2e [INFENG-456] #9244

Merged
merged 5 commits into from
May 22, 2024

Conversation

djanicekpach
Copy link
Contributor

@djanicekpach djanicekpach commented Apr 25, 2024

Ticket

INFENG-456

Description

This adds react E2E tests and page object models for project creation and deletion. This does not complete the the full test we are automating, but it is a self contained test with usable page objects so I'm going to merge it to prevent conflicts later.

Test Plan

CI passes OSS and EE

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.

Copy link

codecov bot commented Apr 25, 2024

Codecov Report

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

Project coverage is 43.99%. Comparing base (860f6a8) to head (99d1e2a).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9244      +/-   ##
==========================================
- Coverage   51.22%   43.99%   -7.23%     
==========================================
  Files         747      423     -324     
  Lines      110720    70801   -39919     
  Branches     2778     2779       +1     
==========================================
- Hits        56718    31152   -25566     
+ Misses      53827    39474   -14353     
  Partials      175      175              
Flag Coverage Δ
harness ?
web 44.35% <0.00%> (-0.01%) ⬇️

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

Files Coverage Δ
webui/react/src/components/ProjectCard.tsx 0.00% <0.00%> (ø)
...t/src/pages/WorkspaceDetails/WorkspaceProjects.tsx 0.00% <0.00%> (ø)

... and 324 files with indirect coverage changes

Copy link

netlify bot commented Apr 25, 2024

Deploy Preview for determined-ui ready!

Name Link
🔨 Latest commit 99d1e2a
🔍 Latest deploy log https://app.netlify.com/sites/determined-ui/deploys/664e0e89eaab9b000836b9e1
😎 Deploy Preview https://deploy-preview-9244--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.

});

static withName<T extends Card>(
Copy link
Contributor

@JComins000 JComins000 Apr 25, 2024

Choose a reason for hiding this comment

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

cool! is this like a second constructor? could we use this syntax, or would that not be the same? just curious

export class CardWithName<T extends Card>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it's similar to a constructor. I think this factory pattern is the cleanest though. I'm open too ideas but I tried a couple options.

I did try making this a second constructor on Card but this doesn't work because it always returns a Card. I don't think new Card(name, type) can actually return a ProjectCard or WorkspaceCard. Maybe it could satisfy the interface based on class shape, but we'd always have to cast it to a more specific type when we work with it which is a pain. If there is clean way to do it I'd prefer this route, but I don't see a good way.

Likewise, I think a new CardWithName<ProjectCard>(name) would return a CardWithName<ProjectCard> class. We could then retrieve an instance of ProjectCard from it but then we are back to a factory pattern with more steps.

I can't make this a constructor on ProjectCard because then it doesn't encapsulate the selector logic so I'd have to re-define that logic for each card type.

Copy link
Contributor

Choose a reason for hiding this comment

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

okay nice. thanks for checking all that! and i think this solution looks clean too. you're doing something pretty different from what i did with tables and rows too.

Copy link
Contributor

Choose a reason for hiding this comment

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

I can't make this a constructor on ProjectCard because then it doesn't encapsulate the selector logic so I'd have to re-define that logic for each card type.

export class CardWithName<T extends Card> extends Card

looks a little ridiculous lol

Copy link
Contributor

@JComins000 JComins000 left a comment

Choose a reason for hiding this comment

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

small nitpick complaints and a question about the generic class. this is looking great!

Copy link
Contributor

@hkang1 hkang1 left a comment

Choose a reason for hiding this comment

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

LGTM!

Comment on lines +48 to +80
// test.afterEach(async ({ page }) => {
// const workspacesPage = new Workspaces(page);

// });

// test('Projects and Workspaces archival and pinning', async ({ page }) => {
// await test.step('Archive a workspace', async () => {});
// await test.step('Unarchive a workspace', async () => {});
// await test.step('Unpin a workspace through the sidebar', async () => {});
// await test.step('Pin a workspace through the sidebar', async () => {});
// await test.step('Archive a project', async () => {});
// await test.step('Unarchive a project', async () => {});
// })
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, guessing just something to add in later.

@djanicekpach djanicekpach force-pushed the djanicek/infeng-456/projects-create branch from be081cd to 2044210 Compare May 21, 2024 21:45
@djanicekpach djanicekpach force-pushed the djanicek/infeng-456/projects-create branch from 30c43f9 to 99d1e2a Compare May 22, 2024 15:25
@djanicekpach djanicekpach merged commit 6fa1420 into main May 22, 2024
83 of 97 checks passed
@djanicekpach djanicekpach deleted the djanicek/infeng-456/projects-create branch May 22, 2024 15:48
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