#4550 validate blueprint options on activation #4602
Conversation
mnholtz
changed the title
…https://github.com/pixiebrix/pixiebrix-extension into feature/4550_validate_blueprint_options_on_activation merge main
src/testUtils/testHelpers.tsx
Outdated
| : ({ children }) => ( | ||
| <MemoryRouter> | ||
| <Provider store={store}>{children}</Provider> | ||
| </MemoryRouter> |
There was a problem hiding this comment.
I don't see a problem with adding this to the wrappers. I'm often having to add this when writing tests and I don't think most of our components would be rendered out of a react router context anyways.
There was a problem hiding this comment.
The extension works without Router, ideally we shouldn't add MemoryRouter to all the tests.
I suggest either
- Add a config parameter to the
createRenderWithWrappers. We create a concrete version of therenderWithWrappersfor each app (PageEditor, Options, and App). Let an app decide if it needsMemoryRouter(or any other wrapper) and add it only for that app.
or - Add
MemoryRouterto the other case a few lines above when we generate<Formik>context. At least this will be more consistent.
Codecov Report
@@ Coverage Diff @@
## main #4602 +/- ##
==========================================
+ Coverage 51.39% 52.19% +0.79%
==========================================
Files 915 915
Lines 27121 27206 +85
Branches 5515 5518 +3
==========================================
+ Hits 13938 14199 +261
+ Misses 12249 12087 -162
+ Partials 934 920 -14
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
src/testUtils/testHelpers.tsx
Outdated
| : ({ children }) => ( | ||
| <MemoryRouter> | ||
| <Provider store={store}>{children}</Provider> | ||
| </MemoryRouter> |
There was a problem hiding this comment.
The extension works without Router, ideally we shouldn't add MemoryRouter to all the tests.
I suggest either
- Add a config parameter to the
createRenderWithWrappers. We create a concrete version of therenderWithWrappersfor each app (PageEditor, Options, and App). Let an app decide if it needsMemoryRouter(or any other wrapper) and add it only for that app.
or - Add
MemoryRouterto the other case a few lines above when we generate<Formik>context. At least this will be more consistent.
| /> | ||
| ); | ||
| await waitForEffect(); | ||
| expect(rendered.asFragment()).toMatchSnapshot(); |
There was a problem hiding this comment.
I think this snapshot is not needed since the happy path is covered by the first test, isn't it?
| @@ -93,7 +133,42 @@ function useWizard(blueprint: RecipeDefinition): [WizardStep[], WizardValues] { | |||
| grantPermissions: false, | |||
| }; | |||
|
|
|||
| return [steps, initialValues]; | |||
| const validationSchema = Yup.object().shape({ | |||
There was a problem hiding this comment.
It might be easier/simpler to write this all as json-schema and then convert it to yup in one pass, instead of going back and forth between json-schema and yup to append the options part and stuff.
There was a problem hiding this comment.
Do you have an example of where we do this elsewhere? I'm not convinced that would be any less complex 🤔
|
When the PR is merged, the first loom link found on this PR will be posted to |
mnholtz
deleted the
feature/4550_validate_blueprint_options_on_activation
branch
What does this PR do?
Reviewer Tips
validationSchemareturn value on theuseWizardhook, and then passing that schema to the Formik form inActivateWizardDiscussion
Demo
https://loom.com/share/96d87c67cdbd4014bfb4167bff6dc4f8
Future Work
Checklist