LG-4799: Code test harnesses
#2697
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ngodb/leafygreen-ui into LG-4799-code-test-harness
🦋 Changeset detectedLatest commit: 4e7bc78 The changes in this PR will be included in the next version bump. This PR includes changesets to release 19 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Size Change: +915 B (+0.07%) Total Size: 1.4 MB
ℹ️ View Unchanged
|
shaneeza
commented
Feb 6, 2025
packages/code/README.md
Outdated
| | Util | Description | Returns | | ||
| | -------------- | ------------------------------------------ | ----------------------- | | ||
| | `getButton()` | Returns the expand button element | `HTMLElement` \| `null` | | ||
| | `isExpanded()` | Returns whether the code block is expanded | `boolean` | |
There was a problem hiding this comment.
I'm not sure if including isExpanded here makes sense. The util is getExpandButton, but isExpanded doesn't really need to be associated with the button since it's testing whether or not the code snippet is expanded or collapsed. Thoughts?
There was a problem hiding this comment.
The expanded state seems like it should be checked for on the Code instance rather than the ExpandButton instance
stephl3
reviewed
Feb 10, 2025
packages/code/README.md
Outdated
Comment on lines
154
to
156
| showLineNumbers={true} | ||
| onCopy={() => {}} | ||
| darkMode={true} |
There was a problem hiding this comment.
might be worth dropping these to narrow focus unless they're being tested for below
packages/code/README.md
Outdated
Comment on lines
161
to
162
| showCustomActionButtons | ||
| customActionButtons={customActionButtons} |
There was a problem hiding this comment.
likewise wondering if these should be dropped to narrow focus. if they're necessary for the copy button, can we explicitly show what's being passed to customActionButtons?
packages/code/README.md
Outdated
Comment on lines
221
to
222
| const testUtils1 = getTestUtils('lg-code-1'); // data-lgid | ||
| const testUtils2 = getTestUtils('lg-code-2'); // data-lgid |
There was a problem hiding this comment.
nit: comments don't seem necessary to me
packages/code/README.md
Outdated
Comment on lines
197
to
201
| import { Tabs, Tab, getTestUtils } from '@leafygreen-ui/tabs'; | ||
|
|
||
| ... | ||
|
|
||
| test('tabs', () => { |
There was a problem hiding this comment.
Suggested change
| import { Tabs, Tab, getTestUtils } from '@leafygreen-ui/tabs'; | |
| ... | |
| test('tabs', () => { | |
| import Code, { getTestUtils, Panel } from '@leafygreen-ui/code'; | |
| ... | |
| test('code', () => { |
packages/code/README.md
Outdated
| | Util | Description | Returns | | ||
| | -------------- | ------------------------------------------ | ----------------------- | | ||
| | `getButton()` | Returns the expand button element | `HTMLElement` \| `null` | | ||
| | `isExpanded()` | Returns whether the code block is expanded | `boolean` | |
There was a problem hiding this comment.
The expanded state seems like it should be checked for on the Code instance rather than the ExpandButton instance
packages/code/src/Code.testutils.tsx
Outdated
Comment on lines
50
to
53
|
|
||
| return { | ||
| ...renderResults, | ||
| }; |
There was a problem hiding this comment.
what do you think about the pattern of calling getTestUtils in the render* test util? then we can do this in the spec file:
const { testUtilName } renderCode();
There was a problem hiding this comment.
I used to do this, but Adam had a point that in the test utils spec file, we should call the test util in each individual test to ensure that it works as expected.
There was a problem hiding this comment.
I don't quite follow. how does that differ from calling it in this helper function? doesn't the getTestUtils still get called in each spec (just wrapped in the render* helper func)?
packages/code/src/Code/Code.spec.tsx
Outdated
|
|
||
| const { getCopyButton } = getTestUtils(); | ||
|
|
||
| expect(getCopyButton().getButton()).toBeInTheDocument(); |
There was a problem hiding this comment.
Couple thoughts here:
-
I think the first util should be named
get*Utilsto more explicitly suggest it's an object of other utils. e.g.getCopyButtonUtils().getButton() -
what do you think about updating this API to be more reusable across different buttons? especially helpful if we need to add test utils for other panel buttons now or in the future
thinking something like:
getButtonUtilsByType('copy').getButton()getButtonUtilsByType('expand').getButton()
There was a problem hiding this comment.
-
I agree with this 👍🏽
-
I like this idea. However, I'm a little worried that consumers might assume they can test custom icon buttons with this util. I'm not testing custom icons because consumers add those with a slot prop.
There was a problem hiding this comment.
I'm also wondering, if we keep getCopyButtonUtils and getExpandButtonUtils, I could simplify getExpandButtonUtils to getExpandButton and return the element and not a util object since this button is never disabled, and no one needs to test that.
packages/code/src/Code/Code.spec.tsx
Outdated
| language: languageOptions[0].displayName, | ||
| panel: ( | ||
| <Panel onChange={() => {}} languageOptions={languageOptions} /> | ||
| ), | ||
| }); | ||
| expect(getByTestId('lg-code-select')).toBeDefined(); | ||
| const { getLanguageSwitcher } = getTestUtils(); |
There was a problem hiding this comment.
similar to above, I think getLanguageSwitcherUtils is a more appropriate name
packages/code/src/Code/Code.types.ts
Outdated
Comment on lines
45
to
50
| /** | ||
| * Determines whether or not the syntax will be rendered in dark mode. | ||
| * | ||
| * @default `false` | ||
| */ | ||
| darkMode?: boolean; |
There was a problem hiding this comment.
nit: could swap with DarkModeProps
|
We discussed this during dev sync, and we're going to try and use other test harnesses inside of this test harness |
…ngodb/leafygreen-ui into LG-4799-code-test-harness
stephl3
reviewed
Feb 13, 2025
| | `getOptionByValue(value: string)` | Returns the option element by its value | `HTMLElement` \| `null` | | ||
| | `isDisabled()` | Returns whether the language switcher is disabled | `boolean` | | ||
|
|
||
| ### CopyButtonUtils |
There was a problem hiding this comment.
did we want any utils for the other panel buttons?
packages/code/src/Code.testutils.tsx
Outdated
Comment on lines
50
to
53
|
|
||
| return { | ||
| ...renderResults, | ||
| }; |
There was a problem hiding this comment.
I don't quite follow. how does that differ from calling it in this helper function? doesn't the getTestUtils still get called in each spec (just wrapped in the render* helper func)?
packages/code/src/Code/Code.spec.tsx
Outdated
| panel: <Panel onChange={() => {}} />, | ||
| }); | ||
| const { getLanguageSwitcherUtils } = getTestUtils(); | ||
| const input = getLanguageSwitcherUtils().getInput(); |
packages/code/src/Code/Code.spec.tsx
Outdated
| panel: <Panel languageOptions={languageOptions} />, | ||
| }); | ||
| const { getLanguageSwitcherUtils } = getTestUtils(); | ||
| const input = getLanguageSwitcherUtils().getInput(); |
There was a problem hiding this comment.
oh i can replace this with _ but I need to call this so that i can test that this test will throw. Do you have other ideas on how to prevent this?
packages/code/src/Code/Code.spec.tsx
Outdated
| panel: <Panel onChange={() => {}} languageOptions={[]} />, | ||
| }); | ||
| const { getLanguageSwitcherUtils } = getTestUtils(); | ||
| const input = getLanguageSwitcherUtils().getInput(); |
| @@ -3,7 +3,7 @@ import { Theme } from '@leafygreen-ui/lib'; | |||
| import { palette } from '@leafygreen-ui/palette'; | |||
| import { color, transitionDuration } from '@leafygreen-ui/tokens'; | |||
|
|
|||
| export const getCopyButtonStyles = ({ | |||
| export const getCopyButtonUtilsStyles = ({ | |||
There was a problem hiding this comment.
is this intentionally renamed or picked up when replacing all?
There was a problem hiding this comment.
ah this was from replace all
| isDisabled: () => boolean; | ||
| } | ||
|
|
||
| export interface LanguageSwitcherUtils { |
There was a problem hiding this comment.
since we're using getTestUtils from the select package now, what do you think about exporting and using the type as well?
I did something similar here for button: https://github.com/mongodb/leafygreen-ui/pull/2724/files#diff-7df5bb9ce245bf9a0263e61b168e928df000ff49720321089b1a0f811c1ded4dR4
| ); | ||
| }; | ||
|
|
||
| const getCopyButtonUtils = () => { |
There was a problem hiding this comment.
did you want to do the same thing as you did for language switcher for this button?
There was a problem hiding this comment.
yes, thanks for catching this
| }; | ||
|
|
||
| const getLanguageSwitcherUtils = () => { | ||
| const testUtils = getSelectTestUtils(LGIDs.select); |
There was a problem hiding this comment.
Something I'm realizing we don't currently support with test harnesses is identifying the correct subcomponent in a given component. If we render multiple Code instances, we can pass in a unique data-lgid which will change the root node's lgid, and we have examples of this in the README files
I don't think we currently support that for the language switchers or the buttons in Code, label/description in TextInput. Is that right?
Since we're already thinking about renaming LGIDs, I'm wondering if we might want to make it a function instead?
constants file
const LGID_ROOT = 'lg_code';
const getLgIds = (root: string) => ({
root,
button: ${root}-button,
...
});
component file
const Component = ({
'data-lgid': dataLgId = LGID_ROOT,
...
}) => {
const lgIds = getLgIds(dataLgId);
...
return (
<RootComponent data-lgid={lgIds.root}>
<Button data-lgid={lgIds.button}>Text</Button>
</RootComponent>
);
Also could take the approach of interpolating. It seems like either might still require passing the dataLgId prop down for nested components, so we should think through that. We can discuss further in dev sync; I think I've got the same issue in Drawer
There was a problem hiding this comment.
I like this, and funnily enough, Kræn just made a ticket about this https://jira.mongodb.org/browse/LG-4898
stephl3
approved these changes
Feb 14, 2025
| /** | ||
| * LGIDs for the code snippet. | ||
| */ | ||
| lgids: GetLgIdsReturnType; |
There was a problem hiding this comment.
Suggested change
| lgids: GetLgIdsReturnType; | |
| lgIds: GetLgIdsReturnType; |
There was a problem hiding this comment.
alternatively, I wonder if Panel could extends LgIdProps as well as opposed to needing to pass this lgIds object through the context
Comment on lines
+254
to
+263
| ### LanguageSwitcherUtils | ||
|
|
||
| | Util | Description | Returns | | ||
| | --------------------------------- | ------------------------------------------------- | ----------------------- | | ||
| | `getInput()` | Returns the language switcher trigger | `HTMLButtonElement` | | ||
| | `getOptions()` | Returns all options in the language switcher | `Array<HTMLElement>` | | ||
| | `getOptionByValue(value: string)` | Returns the option element by its value | `HTMLElement` \| `null` | | ||
| | `isDisabled()` | Returns whether the language switcher is disabled | `boolean` | | ||
|
|
||
| ### CopyButtonUtils |
There was a problem hiding this comment.
Duplicating the select and button utils here means that any time they change in the select or button package, whoever makes said change would have to know to change it here as well to maintain parity. To avoid introducing that maintenance burden, could we instead link to those respective README files?
I did something similar with DrawerTabs: https://github.com/mongodb/leafygreen-ui/pull/2720/files#diff-733f2a760f041bf05129bf53f9f893ce03179ce087309e4b15034d8cf98362d3R69
shaneeza
commented
Feb 18, 2025
|
|
||
| import { | ||
| getCodeStyles, | ||
| getCodeWrapperStyles, | ||
| getCopyButtonWithoutPanelStyles, | ||
| getCopyButtonUtilsWithoutPanelStyles, |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
✍️ Proposed changes
🎟 Jira ticket: Name of ticket
✅ Checklist
For bug fixes, new features & breaking changes
pnpm changesetand documented my changesFor new components
🧪 How to test changes