-
Notifications
You must be signed in to change notification settings - Fork 23
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: Assert Assistant code block #305
Conversation
Simplify automated testing. We mock `react-markdown` due to its ESM-only architecture, so this module cannot be tested through the chat message module.
Co-located third-party manual mocks.
jest.mock( 'strip-ansi', () => ( { | ||
default: jest.fn().mockImplementation( ( str: string ) => str ), | ||
} ) ); |
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.
Relocated to the __mocks__
directory to co-locate third-party mocks and make the mock universal for all tests.
setCliTime( block?.cliTime ?? null ); | ||
} | ||
} | ||
}, [ blocks, cliOutput, content, setCliOutput, setCliStatus, setCliTime ] ); |
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.
blocks
was added addressing the missing dependency lint warning.
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 explored enabling Jest's experimental ESM support to remove the react-markdown
mock (1264f3a, 3121004, 66f9b14). I was successful getting Jest tests to pass, but encountered errors building/running the app itself. So, I pivoted to relocating this logic instead to enable more straightforward unit tests.
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 prefer this disposition of having a separated component. Thanks David for making the change 🙇 !
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.
LGTM 🎊 !
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 prefer this disposition of having a separated component. Thanks David for making the change 🙇 !
Replace erroneous copy and paste with the intended test case.
* refactor: Abstract code block to separate module Simplify automated testing. We mock `react-markdown` due to its ESM-only architecture, so this module cannot be tested through the chat message module. * test: Assert the code "copy" button * test: Assert the code "run" button * test: Assert inline code * test: Assert the "run" button logic * test: Assert code "copy" button * refactor: Relocate manual strip-ansi mock to file-based mock Co-located third-party manual mocks. * test: Assert code block displays past execution output * refactor: Add missing Hook dependency * refactor: Fix typo * test: Fix non-wp-cli code assertion Replace erroneous copy and paste with the intended test case.
Fixes https://github.com/Automattic/dotcom-forge/issues/7698.
Proposed Changes
Refactor the Assistant code block to a separate file to enable automated tests.
This is required as we currently mock
react-markdown
due to its ESM-onlybuild.
Testing Instructions
N/A, no user-facing changes.
Pre-merge Checklist