-
Notifications
You must be signed in to change notification settings - Fork 430
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
test(sanity): ReleaseSummary tests #7870
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
No changes to documentation |
Component Testing Report Updated Nov 25, 2024 8:36 PM (UTC) ✅ All Tests Passed -- expand for details
|
⚡️ Editor Performance ReportUpdated Mon, 25 Nov 2024 20:39:53 GMT
Detailed information🏠 Reference resultThe performance result of
🧪 Experiment resultThe performance result of this branch
📚 Glossary
|
timeout: 5000, | ||
interval: 500, |
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.
@jordanl17 seriously, was the issue the whole time just a timeout? 🥲
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.
Not quite - this seemed to be some flakiness in the tests, and I'm not quite sure why. Adding in a longer timeout seems to resolve the flake, but there were quite a few other parts necessary to actually get these tests to work:
- awaiting the
waitFor
for the spinner to go away and the table to render - Updating the mock for
DefaultPreview
to pass the props through - Something else that I can't remember...
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.
God, this is so frustrating 😭 😂
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.
Looks good! Added some comments/questions you can consider, but feel free to merge.
useBundleDocumentsMockReturnWithResults, | ||
} from './__mocks__/useBundleDocuments.mock' | ||
|
||
vi.mock('../../../index', () => ({ |
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.
could we be more specific about the path here? I think it's preferable to mock the file where the function is defined, e..g where useDocumentPresence
is actually defined. Same with useDocumentPreviewStore
. That would ensure these gets mocked also in a (potential future case) where these are imported from a more specific internal path.
}), | ||
useDocumentPreviewStore: vi.fn().mockReturnValue({ | ||
unstable_observeDocumentIdSet: vi.fn(() => ({ | ||
pipe: vi.fn(), |
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.
Could the unstable_observeDocumentIdSet
mock instead return an observable? It's not clear to me what mocking pipe()
here does 🤔
vi.mock('../../../../preview/components/_previewComponents', async () => { | ||
return { | ||
_previewComponents: { | ||
default: vi.fn((arg) => <DefaultPreview {...arg} />), |
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.
Could this be a dummy component or are we asserting anything "default preview" specific?
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.
We are asserting on the passing through and rendering of the document preview itself. this mock only exists as the _previewComponents
has an as yet unknown quirk that during test runs all the exports are set to undefined
rather than their correct components
Description
What to review
Testing
Notes for release