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.

Sign up for GitHub

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

Jump to bottom

[UI v2] test: Adds tests for global concurrency view #16333

Merged
merged 4 commits into from
Dec 12, 2024
Merged

[UI v2] test: Adds tests for global concurrency view #16333

merged 4 commits into from
Dec 12, 2024

Conversation

devinvillarosa
Copy link
Contributor

@devinvillarosa devinvillarosa commented Dec 11, 2024 •
edited
Loading

  1. Adds unit tests to global concurrency limit components
  2. Reorganizes some files for simplicity
  3. Abstracts data tables to own functional components for isolated testing

Checklist

  • This pull request references any related issue by including "closes <link to issue>"
    • If no issue exists and your change is not a small fix, please create an issue first.
  • If this pull request adds new functionality, it includes unit tests that cover the changes
  • If this pull request removes docs files, it includes redirect settings in mint.json.
  • If this pull request adds functions or classes, it includes helpful docstrings.

Relates to #15512

@devinvillarosa devinvillarosa force-pushed the add-global-concurrency-tests branch 2 times, most recently from 90c0d01 to a077cbf Compare December 11, 2024 06:44
@devinvillarosa devinvillarosa marked this pull request as ready for review December 11, 2024 06:44
@devinvillarosa devinvillarosa force-pushed the add-global-concurrency-tests branch 4 times, most recently from e9b6de5 to e498db2 Compare December 11, 2024 07:31
@devinvillarosa devinvillarosa marked this pull request as draft December 11, 2024 16:24
@devinvillarosa devinvillarosa force-pushed the add-global-concurrency-tests branch 7 times, most recently from 66f007b to 7c69e16 Compare December 11, 2024 19:17
@github-actions github-actions bot added the ui-replatform Related to the React UI rewrite label Dec 11, 2024
@devinvillarosa devinvillarosa force-pushed the add-global-concurrency-tests branch 3 times, most recently from 5ad6562 to b343140 Compare December 11, 2024 19:40
@devinvillarosa devinvillarosa force-pushed the add-global-concurrency-tests branch 5 times, most recently from 8d6d63d to 793b837 Compare December 11, 2024 19:59
devinvillarosa
devinvillarosa commented Dec 11, 2024
View reviewed changes
ui-v2/src/components/concurrency/task-run-concurrenct-view/dialogs/create-dialog.test.tsx Outdated
active_slots: [] as Array<string>,
};

test.skip("CreateLimitDialog calls onSubmit upon entering form data", async () => {
Copy link
Contributor Author

@devinvillarosa devinvillarosa Dec 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Skipping this test for now.
For some reason it passes on my machine, but fails in CI

Screenshot 2024-12-11 at 12 00 54 PM

Screenshot 2024-12-11 at 11 59 03 AM

Copy link
Member

Choose a reason for hiding this comment

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

Could it be because the ResizeObserverMock isn't in a beforeAll?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that didn't fix it.
I'm pretty stumped on this one

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that's really weird. Especially considering the tests for the other dialogs are fine 🫠

@devinvillarosa devinvillarosa marked this pull request as ready for review December 11, 2024 20:04
@devinvillarosa devinvillarosa force-pushed the add-global-concurrency-tests branch from 793b837 to 5c81614 Compare December 11, 2024 20:32
@devinvillarosa devinvillarosa force-pushed the add-global-concurrency-tests branch from 5c81614 to 3d5bd60 Compare December 11, 2024 20:48
desertaxle
desertaxle reviewed Dec 11, 2024
View reviewed changes
...bal-concurrency-view/dialog/create-or-edit-limit-dialog/create-or-edit-limit-dialog.test.tsx
await user.click(screen.getByRole("button", { name: /save/i }));

// ------------ Assert
expect(mockOnSubmitFn).toHaveBeenCalledOnce();
Copy link
Member

Choose a reason for hiding this comment

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

Can you assert that this has been called with the expected values?

Copy link
Contributor Author

@devinvillarosa devinvillarosa Dec 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I've updated the other calls to validate to be called with expected values.

This one is just a UI callback (in this case onSubmit is just a callback to close the dialog)

Copy link
Member

Choose a reason for hiding this comment

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

Gotcha, I guess I was hoping for a test verifying the form values make it to the API call correctly, but maybe that's a better PlayWright test.

...bal-concurrency-view/dialog/create-or-edit-limit-dialog/create-or-edit-limit-dialog.test.tsx
await user.click(screen.getByRole("button", { name: /update/i }));

// ------------ Assert
expect(mockOnSubmitFn).toHaveBeenCalledOnce();
Copy link
Member

Choose a reason for hiding this comment

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

Can you assert that this has been called with the expected values?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as comment above

ui-v2/src/components/concurrency/global-concurrency-view/data-table/data-table.test.tsx Show resolved Hide resolved
ui-v2/src/components/concurrency/task-run-concurrenct-view/dialogs/create-dialog.test.tsx Outdated
active_slots: [] as Array<string>,
};

test.skip("CreateLimitDialog calls onSubmit upon entering form data", async () => {
Copy link
Member

Choose a reason for hiding this comment

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

Could it be because the ResizeObserverMock isn't in a beforeAll?

@devinvillarosa devinvillarosa marked this pull request as draft December 11, 2024 21:21
@devinvillarosa devinvillarosa force-pushed the add-global-concurrency-tests branch from 885a60a to 4c9e55d Compare December 11, 2024 22:29
@devinvillarosa devinvillarosa marked this pull request as ready for review December 11, 2024 22:34
@devinvillarosa devinvillarosa force-pushed the add-global-concurrency-tests branch from 89b6de8 to bfae1f3 Compare December 11, 2024 22:43
@devinvillarosa devinvillarosa force-pushed the add-global-concurrency-tests branch from bfae1f3 to 52bd8d0 Compare December 11, 2024 22:57
desertaxle
desertaxle approved these changes Dec 12, 2024
View reviewed changes
Copy link
Member

@desertaxle desertaxle left a comment

Choose a reason for hiding this comment

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

LGTM!

@devinvillarosa devinvillarosa merged commit da5439d into main Dec 12, 2024
8 checks passed
@devinvillarosa devinvillarosa deleted the add-global-concurrency-tests branch December 12, 2024 16:06
EmilRex pushed a commit that referenced this pull request Dec 13, 2024
@devinvillarosa @EmilRex
[UI v2] test: Adds tests for global concurrency view (#16333)
cdfa773
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@desertaxle desertaxle desertaxle approved these changes

@aaazzam aaazzam Awaiting requested review from aaazzam aaazzam is a code owner

@cicdw cicdw Awaiting requested review from cicdw cicdw is a code owner

@zzstoatzz zzstoatzz Awaiting requested review from zzstoatzz zzstoatzz is a code owner

Assignees
No one assigned
Labels
ui-replatform Related to the React UI rewrite
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@devinvillarosa @desertaxle