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

[Draft] using react-testing-library to test useGroups hook #1530

Closed
wants to merge 4 commits into from

Conversation

christianvogt
Copy link
Contributor

@christianvogt christianvogt commented Jul 17, 2023

Based on PR #1528 - creating unit tests for useGroups hook.

Tests using react-testing-library

Commit: 46fa36d
npm run test:jest useGroups

Used jest.mock to mock the k8s api from @openshift/dynamic-plugin-sdk-utils. It's likely possible to instead use mock service worker (the utility used by our storybook tests) with jest.

Tests using storybook & playwright

Commit: c0b270b
npx playwright test ./src/__tests__/integration/hooks/useGroups.spec.ts

These tests do not assert whether the hook performs a 2nd network request after receiving a 403 error. Not sure how this can be asserted. Mock service worker discourages such assertions.

Further investigations required:

  • Why arbitrary 2s waits are required => Could use specific selectors instead to wait till present.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 17, 2023

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign andrewballantyne for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the needs-ok-to-test The openshift bot needs to label PRs from non members to avoid strain on the CI label Jul 17, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 17, 2023

Hi @christianvogt. Thanks for your PR.

I'm waiting for a opendatahub-io member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@manaswinidas
Copy link
Contributor

/ok-to-test

@openshift-ci openshift-ci bot added ok-to-test The openshift bot needs `ok-to-test` to allow non member PRs to run the tests. and removed needs-ok-to-test The openshift bot needs to label PRs from non members to avoid strain on the CI labels Jul 18, 2023
@lucferbux
Copy link
Contributor

Just a few tips before readying this branch:

  1. I think is really great if we add react-testing-library to our efforts, is a great framework to help us out with our integration testing. We can include those changes in the pr that @Gkrumbach07 has raised here or do a follow up once it's merged: Testing arch added to docs #1481
  2. I see one commit is based on the changes of https://github.com/opendatahub-io/odh-dashboard/pull/1528/files, we can now discard that commit and just rebase from main
  3. We've recently changed our mergin strategy, and we'll enforce keep clean PRs, meaning that we are aiming for one commit per PR, if that fits these changes, please squash the git history to fit that strategy.

Otherwise the changes look really great, I'm looking forward to keeping on improving our testing efforts!

@christianvogt
Copy link
Contributor Author

@lucferbux Thanks for the insights.
I should have mentioned this in the description. This PR was posted as a draft just for discussion purposed and not intended to go in as is. @Gkrumbach07 and I had a conversation about testing we'll be doing some followup work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ok-to-test The openshift bot needs `ok-to-test` to allow non member PRs to run the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants