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

(HATCH-478) implement react-ui tests #447

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

(HATCH-478) implement react-ui tests #447

wants to merge 4 commits into from

Conversation

snoattoh
Copy link
Contributor

@snoattoh snoattoh commented Feb 23, 2024

Description

This brings the test coverage for the react-ui package up to 90%.
Note, a good amount of the react-ui tests have been smoke tests since the restClient (or dataSource) and the hatchedReact components bring the elements together.

There are still tests that could use more coverage, but I've opted to prioritize other work rather than exceeding a sufficient coverage.

Jira ticket links

https://bitovi.atlassian.net/browse/HATCH-478

Test Plan

  1. Navigate to the react-ui directory
  2. npm run test:coverage
  3. Make sure there are no errors, but also review the code of course, some of the tests could be misguided.

Definition of done

  • Does new code have 90% testing coverage (100% for core) of both unit and E2E tests?
  • Is code secure? If applicable, add security notes to the description.
  • Do all new TODO comments have Jira links with enough information?
  • Were all changes published and deployed to the grid?
  • Has the browser error-console been reviewed to not contain new errors introduced by these code changes?
  • The site looks “good” above 1000px width.
  • The site looks “good” when the window height is small. No double scrollbars.
  • Were the changes tested to ensure 508 compliance?

deleteOne: () => Promise.resolve(),
})

it("Works", async () => {

Choose a reason for hiding this comment

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

I'd like to see more descriptive names than "Works". What works? Is it that the Data Grid renders"? (apply to all)

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've updated the ones in the scope of my changes, should I update all of the one's in react-ui's scope or it'd be better to perhaps make a ticket for the repo?

Choose a reason for hiding this comment

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

I'll let @nlundquist weigh in regarding priority of this vs. other items.

Copy link
Collaborator

Choose a reason for hiding this comment

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

go ahead and improve these names

expect(screen.getByLabelText("second"))
})

// it("Number works", async () => {

Choose a reason for hiding this comment

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

are we leaving in commented out code for a reason?

Copy link
Collaborator

Choose a reason for hiding this comment

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

i agree any skipped tests should be verified and either have their implementations corrected or removed as needed

@roy-coder
Copy link
Collaborator

Can we please enforce minimal code coverage like we did in the core package so we won't need to do it again?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants