-
Notifications
You must be signed in to change notification settings - Fork 178
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
Storybook test integration #974
Storybook test integration #974
Conversation
/retest |
I have kept the old tests functioning, however we may want to move them over to storybooks if they are rendering in any way |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking awesome! Thanks for doing this up Gage! I have some comments and questions.
Also, is there anyway we can get the test not to require me to start an instance? Just for ease of use -- could it start one and then use it?
frontend/__mocks__/mockWorkbench.ts
Outdated
@@ -0,0 +1,113 @@ | |||
// /api/k8s/apis/route.openshift.io/v1/namespaces/project/routes/workbench | |||
|
|||
export const mockWorkbench = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mock ... Route?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
its a mock for a route GET 200 response. It uses mock service worker to apply the mocks to the network calls. I found it makes it easier to control what is going on without having to put mocks on every function that calls the api
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You called it mockWorkbench
but it returns a Route object
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that is whatever was returned by that api route commented above the file. It was called so I mocked it essentially, didn't look too much into why
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I imagine it was called that because the API is broken up like this:
// /api/k8s/apis/route.openshift.io/v1/namespaces/project/routes/workbench
/api/k8s
-- our internal route to our backend/apis
-- CR endpoint for K8s (vs/api
for native k8s endpoints)/route.openshift.io
-- the group for the API/v1
-- the version of the group API/namespaces
-- it's a namespaced resource, next property is the namespace/project
-- the name of your namespace/routes
-- the plural of the resource in the group/version API/workbench
-- your name for the route (in this case, named by your workbench you created, Kubeflow just uses the same workbench name as all the related resources for name collision reasons)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My question is -- mock<X>
is required by the mocker? Where <X>
is the last part of the API call?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is not. that is just a name i gave it. I can do something like mock<resource_type><name>
export const CreateProject = Template.bind({}); | ||
CreateProject.parameters = { | ||
a11y: { | ||
element: '.pf-c-backdrop', | ||
}, | ||
}; | ||
CreateProject.play = async ({ canvasElement }) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting -- going to need a crash course on what's going on here. Got links to docs to explain what is happening?
Since this is a common pattern, did we want a utility to setup a test?
Template.bind
feels ... old. This is just a hack to duplicating an instance so you can mutate it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://storybook.js.org/docs/6.5/react/writing-stories/args/
This talks about how a story is constructed.
TLDR
The template is reused for each story.
Parameters are basically only
Used for a11y root html element setup... b/c modals pop out of the root element
Play is the function that runs for testing.
I can do a small docs on this too if that helps people use it
added a mock and decorator added tests fixed deps for storybook linter old tests dont work on jest 28, reverting linter error fix fix build errors fixed deps
fixed tests
There is but the solution required 3 more dev deps (to run the command concurrently and wait for it to load, then tear it down). So i did not add that feature, but can if those deps are fine with you |
@Gkrumbach07 nah -- lets follow up on it. No worries! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nits for consistency... let me test this PR out again.
@@ -0,0 +1,39 @@ | |||
export const mockAPINamespaceProjects = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't there be a comment at the top of this file for the api path?
@@ -0,0 +1,140 @@ | |||
export const mockDashboardConfig = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely great work here. There are a couple things left to tidy up -- but this is not going live today. It's a start, a foundation -- others can start looking at integrating it in with their work.
Two remaining issues we should look back at:
- The missing doc comments about the mocks and api paths
- The tests fail on a11y -- this might be fixed by the work Jenny is doing.
Let's get this into the repo and iterate on it. I'll make sure to log something to enable it on PRs -- in which case we will have to make the tests pass. Always more to do 😄
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andrewballantyne The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
more workbenches a11y updates fix dsp modal fix more select issues prettier Add faker for the pass through api to enable regular user testing (opendatahub-io#806) * Add faker for the pass through api to enable regular user testing * Add faker for backend openshift user * add impersonate function for developers * address comments and modify the doc * Update to get the access token by making API call * update doc * add error message * Fix lint issues * address comments --------- Co-authored-by: Juntao Wang <juntwang@redhat.com> Storybook test integration (opendatahub-io#974) * added storybook boilerplate added a mock and decorator added tests fixed deps for storybook linter old tests dont work on jest 28, reverting linter error fix fix build errors fixed deps * trimmed mocks fixed tests * fix webpack changes * added a11y test to test-runner fix a11y errors in settings (opendatahub-io#979) varname rename bodyText to ariaLabel add aria label to select add aria-label to Select fix a11y violation in cluster storage modal and list view
more workbenches a11y updates fix dsp modal fix more select issues prettier Add faker for the pass through api to enable regular user testing (opendatahub-io#806) * Add faker for the pass through api to enable regular user testing * Add faker for backend openshift user * add impersonate function for developers * address comments and modify the doc * Update to get the access token by making API call * update doc * add error message * Fix lint issues * address comments --------- Co-authored-by: Juntao Wang <juntwang@redhat.com> Storybook test integration (opendatahub-io#974) * added storybook boilerplate added a mock and decorator added tests fixed deps for storybook linter old tests dont work on jest 28, reverting linter error fix fix build errors fixed deps * trimmed mocks fixed tests * fix webpack changes * added a11y test to test-runner fix a11y errors in settings (opendatahub-io#979) varname rename bodyText to ariaLabel add aria label to select add aria-label to Select fix a11y violation in cluster storage modal and list view
more workbenches a11y updates fix dsp modal fix more select issues prettier Add faker for the pass through api to enable regular user testing (opendatahub-io#806) * Add faker for the pass through api to enable regular user testing * Add faker for backend openshift user * add impersonate function for developers * address comments and modify the doc * Update to get the access token by making API call * update doc * add error message * Fix lint issues * address comments --------- Co-authored-by: Juntao Wang <juntwang@redhat.com> Storybook test integration (opendatahub-io#974) * added storybook boilerplate added a mock and decorator added tests fixed deps for storybook linter old tests dont work on jest 28, reverting linter error fix fix build errors fixed deps * trimmed mocks fixed tests * fix webpack changes * added a11y test to test-runner fix a11y errors in settings (opendatahub-io#979) varname rename bodyText to ariaLabel add aria label to select add aria-label to Select fix a11y violation in cluster storage modal and list view
more workbenches a11y updates fix dsp modal fix more select issues prettier Add faker for the pass through api to enable regular user testing (opendatahub-io#806) * Add faker for the pass through api to enable regular user testing * Add faker for backend openshift user * add impersonate function for developers * address comments and modify the doc * Update to get the access token by making API call * update doc * add error message * Fix lint issues * address comments --------- Co-authored-by: Juntao Wang <juntwang@redhat.com> Storybook test integration (opendatahub-io#974) * added storybook boilerplate added a mock and decorator added tests fixed deps for storybook linter old tests dont work on jest 28, reverting linter error fix fix build errors fixed deps * trimmed mocks fixed tests * fix webpack changes * added a11y test to test-runner fix a11y errors in settings (opendatahub-io#979) varname rename bodyText to ariaLabel add aria label to select add aria-label to Select fix a11y violation in cluster storage modal and list view
more workbenches a11y updates fix dsp modal fix more select issues prettier remove unused import revert id update fix project modal wip Workbenches, need image more workbenches a11y updates fix dsp modal fix more select issues prettier Add faker for the pass through api to enable regular user testing (opendatahub-io#806) * Add faker for the pass through api to enable regular user testing * Add faker for backend openshift user * add impersonate function for developers * address comments and modify the doc * Update to get the access token by making API call * update doc * add error message * Fix lint issues * address comments --------- Co-authored-by: Juntao Wang <juntwang@redhat.com> Storybook test integration (opendatahub-io#974) * added storybook boilerplate added a mock and decorator added tests fixed deps for storybook linter old tests dont work on jest 28, reverting linter error fix fix build errors fixed deps * trimmed mocks fixed tests * fix webpack changes * added a11y test to test-runner fix a11y errors in settings (opendatahub-io#979) varname rename bodyText to ariaLabel add aria label to select add aria-label to Select fix a11y violation in cluster storage modal and list view package-lock.json package-lock.json update DetailsSection.tsx remove aria-labelledby Table PR suggestion from Gage lint and revert Stack removal
more workbenches a11y updates fix dsp modal fix more select issues prettier remove unused import revert id update fix project modal revert stack wip Workbenches, need image more workbenches a11y updates fix dsp modal fix more select issues prettier Add faker for the pass through api to enable regular user testing (opendatahub-io#806) * Add faker for the pass through api to enable regular user testing * Add faker for backend openshift user * add impersonate function for developers * address comments and modify the doc * Update to get the access token by making API call * update doc * add error message * Fix lint issues * address comments --------- Co-authored-by: Juntao Wang <juntwang@redhat.com> Storybook test integration (opendatahub-io#974) * added storybook boilerplate added a mock and decorator added tests fixed deps for storybook linter old tests dont work on jest 28, reverting linter error fix fix build errors fixed deps * trimmed mocks fixed tests * fix webpack changes * added a11y test to test-runner fix a11y errors in settings (opendatahub-io#979) varname rename bodyText to ariaLabel add aria label to select add aria-label to Select fix a11y violation in cluster storage modal and list view package-lock.json package-lock.json fix a11y in data connections and models/model servers update DetailsSection to match changes on other branch update Table file fix pagination axe error PR feedback from Andrew change aria label text on password input
This reverts commit 1a730eb.
* added storybook boilerplate added a mock and decorator added tests fixed deps for storybook linter old tests dont work on jest 28, reverting linter error fix fix build errors fixed deps * trimmed mocks fixed tests * fix webpack changes * added a11y test to test-runner
* added storybook boilerplate added a mock and decorator added tests fixed deps for storybook linter old tests dont work on jest 28, reverting linter error fix fix build errors fixed deps * trimmed mocks fixed tests * fix webpack changes * added a11y test to test-runner
fixes: #962
Configured storybooks for testing in the future. I added MSW add on, accessibly add on, and test-runner addon.
Description
You can now generate a local dev storybook by running
npm run storybook
in the frontend folder. You can navigate through each component and select stories. A story in our case will represent a user flow we want to test.For now the example is looking at the entire project page defined in
ProjectView.tsx
. The stories in there include case for creating, editing, and deleting.Each story has interaction tests and a11y accessibility tests.
You can run the interaction tests in the terminal with
npm run test:storybook
inside the frontend folder. The backend is not connected for storybooks as it is only looking at frontend tests. So there are mocks created for most of theGET
requests.How Has This Been Tested?
To test run
npm run test:storybook
in the frontend folder. All tests should pass.To see storybook run
npm run storybook
in frontend folder. data should populate. (note going to other routes will not work)Merge criteria: