test: Djanicek/infeng 456/workspaces and projects #9117
Conversation
✅ Deploy Preview for determined-ui ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
djanicekpach
changed the title
| * @param {string} [obj.selector] - Used instead of `defaultSelector` | ||
| */ | ||
| export class ModelRegistryComponent extends NamedComponent { | ||
| override defaultSelector: string = '[data-testid=modelRegistry]'; |
There was a problem hiding this comment.
override not needed here
|
|
||
| /** | ||
| * Returns a representation of the Model Registry Page component. | ||
| * This constructor represents the contents in src/components/Page.tsx. |
There was a problem hiding this comment.
gotta update the docstring here too.
this class wants to have a Page like SignIn page has, and then it should import ModelRegistry from e2e/models/component/ModelRegistry.ts.
There was a problem hiding this comment.
The ModelRegistry component will be initialize with pageComponent as it's parent
There was a problem hiding this comment.
this is not the page object, it is the page component. It's not referenced yet but will be in the next PR. It will be wrapped by the ModelRegistry page and also the Pivot n workspace details.
There was a problem hiding this comment.
Oh I see. Please remove the Page from that filename
There was a problem hiding this comment.
and doublecheck the docstring
| export class WorkspacesList extends NamedComponent { | ||
| override defaultSelector: string = '[id=workspaces]'; | ||
| readonly newWorkspaceButton: BaseComponent = new BaseComponent({ | ||
| parent: this, |
There was a problem hiding this comment.
same here, you want a PageComponent parent
| export class WorkspaceCreateModal extends Modal { | ||
|
|
||
| readonly workspaceName: BaseComponent = new BaseComponent({ | ||
| parent: this, |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9117 +/- ##
===========================================
- Coverage 46.03% 35.37% -10.67%
===========================================
Files 768 455 -313
Lines 106731 67574 -39157
Branches 2429 2438 +9
===========================================
- Hits 49133 23903 -25230
+ Misses 57375 43439 -13936
- Partials 223 232 +9
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
|
||
| test.afterEach(async ({ page }) => { | ||
| const workspacesPage = new Workspaces(page); | ||
| await test.step('Delete a workspace', async () => { |
There was a problem hiding this comment.
best not to have split behavior in after all.
i would say the afterAll should delete any workspace through a single method. if we want to test an alternate method for deleting workspaces, then we should perform that in a test case. Check out how I deactivate users at the end of user tests.
There was a problem hiding this comment.
best not to have split behavior in after all.
Why?
I need to test both ways of deleting. It's explicit in the test plan. I actually kind of like having steps in after all as clean-up, but I'm planning to defer the big test-organizational stuff until a future PR.
There's a trade off here, we can split everything into separate test cases which is cleaner, but also slower and more duplicative since we need to create/delete more workspaces overall. This test case needs 2 workspaces, so doing create/delete differently with them both to get the free coverage is better IMO than adding an additional test case to get the same coverage. I suspect at the end I may need to split this into separate cases. It's a big test. But for now I'm planning to leave it to see if we can have that coverage without more test cases.
There was a problem hiding this comment.
intuitively, I would think if teardown should be the most stable and straightforward part of the test. i.e., if it fails on one test, i would expect it to fail in every test. if i'm just running a single test, it could be confusing which teardown is running. if we want two teardowns, maybe we should have two describes. anyway, this is all semantics and ultimately these details dont matter so much. I'll leave the decision to you
djanicekpach
changed the title
djanicekpach
changed the title
djanicekpach
force-pushed
the
djanicek/infeng-456/workspaces-and-projects
branch
from
63abd4b
to
5142c95
Compare
|
Build docs is failing after rebase because it's failing on master https://hpe-aiatscale.slack.com/archives/C04C9JXB1C2/p1713280926191379 |
djanicekpach
force-pushed
the
djanicek/infeng-456/workspaces-and-projects
branch
from
5142c95
to
58caec2
Compare
Test: Djanicek/infeng 456/workspaces and projects
test: ui e2e workspace create and delete
Ticket
INFENG-456
Description
adds workspace create and delete tests to playwright. Requires a hew change to work correctly.
Test Plan
Checklist
docs/release-notes/.See Release Note for details.