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

Add storybook testing setup for client #8335

Draft
wants to merge 24 commits into
base: develop
Choose a base branch
from
Draft

Conversation

rikukissa
Copy link
Member

@rikukissa rikukissa commented Jan 15, 2025

image

Copy link

Oops! Looks like you forgot to update the changelog. When updating CHANGELOG.md, please consider the following:

  • Changelog is read by country implementors who might not always be familiar with all technical details of OpenCRVS. Keep language high-level, user friendly and avoid technical references to internals.
  • Answer "What's new?", "Why was the change made?" and "Why should I care?" for each change.
  • If it's a breaking change, include a migration guide answering "What do I need to do to upgrade?".

@naftis
Copy link
Collaborator

naftis commented Jan 15, 2025

Would we migrate the current stuff from the components side or maintain both going forward?

Copy link
Collaborator

@makelicious makelicious left a comment

Choose a reason for hiding this comment

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

Looks good, suggested some actual test case to try out

packages/client/src/v2-events/features/events/fixtures.ts Outdated Show resolved Hide resolved
@@ -263,3 +284,20 @@ export const tennisClubMembershipEvent = {
}
]
} satisfies EventConfig

export const tennisClubMembershipEventIndex: EventIndex = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We could use similar pattern as we have on
packages/events/src/tests/generators.ts.

async () => {
await worker.start()
}
],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe test for sorting and that pagination works (currently doesn't 😬 )

import React from 'react'
import superjson from 'superjson'

const trpcMsw = createTRPCMsw<AppRouter>({
Copy link
Collaborator

Choose a reason for hiding this comment

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

What a casing

@@ -90,7 +90,7 @@ export default defineConfig(({ mode }) => {
},
resolve: {
alias: {
crypto: 'crypto-js'
crypto: require.resolve('crypto-js')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we know why we have this? :D

Copy link
Member Author

Choose a reason for hiding this comment

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

I did try and figure it out but gave up

Copy link
Collaborator

Choose a reason for hiding this comment

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

Feel free to remove if E2E's pass imo

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think the dev environment starts without this

Copy link
Collaborator

@makelicious makelicious left a comment

Choose a reason for hiding this comment

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

Good stuff, thanks! some clean up comments

baseUrl: '/api/events',
transformer: { input: superjson, output: superjson }
})
console.log(
Copy link
Collaborator

Choose a reason for hiding this comment

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

can be cleaned // lint-ignore

tRPCMsw.event.config.get.query(() => {
return [tennisClubMembershipEvent]
}),
tRPCMsw.events.get.query(() => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I updated the api so event.list replaces events.get. If not, I have made a mistake

Copy link
Member Author

Choose a reason for hiding this comment

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

No, I think it's just that doesn't yet support tRPC 11 types maloguertin/msw-trpc#41

)

export const handlers = {
events: [
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would be cool if events handlers lived under events

systemRoles: [
graphql.query('getSystemRoles', () => {
return HttpResponse.json({
data: {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we utilise separate fixture files as we have in the past, or come up with some other organisation principle? +1000 files with static code are quite hard to navigate

})
],
locations: [
http.get('http://localhost:7070/location', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we use envs here

Copy link
Member Author

Choose a reason for hiding this comment

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

True 🤔 best would be if all of these request paths would be behind /api so it would be a non-issue

})
],
forms: [
http.get('http://localhost:2021/forms', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

usage through envs would be super

import { System, SystemStatus, SystemType } from '@client/utils/gateway'
import type {
Facility,
CRVSOffice,
AdminStructure
} from '@client/offline/reducer'
import forms from './forms.json'
import languages from './languages.json' assert { type: 'json' }
Copy link
Collaborator

Choose a reason for hiding this comment

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

never seen assert 🤯

title: 'OfficeHome',
component: OfficeHome,
decorators: [(Story) => <Story />]
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

satisfies Meta<typeof OfficeHome>might work


export default meta

type Story = StoryObj<any>
Copy link
Collaborator

Choose a reason for hiding this comment

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

type Story = StoryObj<typeof meta>

handlers: {
registrationHome: [
graphql.query('registrationHome', () => {
return HttpResponse.json({
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could this utilise the ones in default-request-handlers? Share fixtures or anything like that?

@rikukissa rikukissa force-pushed the events-v2-storybook branch 2 times, most recently from e483238 to 6159edf Compare January 17, 2025 11:33
@rikukissa rikukissa force-pushed the events-v2-storybook branch 3 times, most recently from 6db4c42 to 472d665 Compare January 17, 2025 12:01
@rikukissa rikukissa force-pushed the events-v2-storybook branch from 472d665 to 01fd9a9 Compare January 17, 2025 12:09
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.

3 participants