test(common): add prop theme utility for E2E #9027
Conversation
| * Helper to test custom theming of a component's associated tokens. | ||
| * | ||
| * @example | ||
| * describe("default", () => { |
There was a problem hiding this comment.
Is the default name intentional? All other utils use a name closer to the test subject.
| { | ||
| selector: string; | ||
| shadowSelector?: string; | ||
| targetProp: string; |
There was a problem hiding this comment.
Can you type this as keyof CSSStyleDeclaration?
| ): void { | ||
| it("is theme-able", async () => { | ||
| const page = await simplePageSetup(componentTagOrHTML); | ||
| const expectChecklist: Record< |
There was a problem hiding this comment.
Can we add explicit types for these objects?
| } | ||
| >, | ||
| ): void { | ||
| it("is theme-able", async () => { |
| targetProps[token] = [targetProp, themedTokenValue, state]; | ||
| } | ||
|
|
||
| // let all page.find calls resolve |
There was a problem hiding this comment.
This isn't quite accurate, find does not rely on waitForChanges.
| await page.mouse.down(); | ||
| } | ||
| if (stateName === "focus") { | ||
| await page.mouse.up(); |
There was a problem hiding this comment.
Should this also call mouse.down or does this depend on press being specified before? If it's the latter, it might be prone to being missed.
There was a problem hiding this comment.
thanks for the catch
| for (const [targetProp, themedTokenValue, state] of props) { | ||
| if (state) { | ||
| if (typeof state === "string") { | ||
| await target[state](); |
There was a problem hiding this comment.
Can we use a type to hint at which E2EElement methods are used here?
There was a problem hiding this comment.
refactored this so it's simplified and more clear
| * | ||
| * @param page - the e2e page | ||
| * @param target - the element to get the computed style from | ||
| * @param selector - the selector of the target element |
There was a problem hiding this comment.
selector can be removed as there is no matching prop in the function signature.
| export async function assertThemedProps( | ||
| page: E2EPage, | ||
| target: E2EElement, | ||
| props: [string, string, string | Record<string, { attribute: string; value: string }> | undefined][], |
There was a problem hiding this comment.
Is the type accurate? I'm not sure I spot where the 3rd item (Record<string, { attribute: string; value: string }> is provided in themed. The doc mentions a second value, but there isn't a mention of the third one.
On a related note, can we use an object vs a tuple? It's not clear from the type itself what the tuple contains. Alternatively, props could be renamed to make this clearer. It's slightly misleading as it has both the prop and value.
There was a problem hiding this comment.
yeah this was silly. Refactored to be much more simple and clear.
| for (let i = 0; i < shadowSelectors.length; i++) { | ||
| const s = shadowSelectors[i]; | ||
|
|
||
| if (i === 0) { |
There was a problem hiding this comment.
I'm not sure I follow this. Could you shed some light on how this works?
Also, has custom deep piercing been working smoothly? I recall this wasn't working before due to a Stencil bug that was recently fixed.
There was a problem hiding this comment.
This uses the Stencil bug work around that you posted. It does expect a certain pattern to the shadowSelector but it does work.
[shadowSelector] [wrapper-class] [sub-element] >>> [wrapper-class] [sub-element] >>> [shadowSelector]
| for (const token in tokens) { | ||
| let selectors = tokens[token]; | ||
|
|
||
| if (!isArray(selectors)) { |
There was a problem hiding this comment.
Can you use Array.isArray instead of adding a custom function for this?
There was a problem hiding this comment.
no. The custom type guard is required. See microsoft/TypeScript#19892
There was a problem hiding this comment.
Are you seeing incorrect types or a particular TS error? The issue you referenced is related to ReadOnlyArray type, which we are not using.
Types seem fine in both code paths.
There was a problem hiding this comment.
That doesn't work for me. It says it doesn't know if it's a string or an array.
| * @param options.state - the state to apply to the target element | ||
| * @param options.expectedValue - the expected value of the targetProp | ||
| */ | ||
| export async function assertThemedProps(page: E2EPage, options: TestTarget): Promise<void> { |
There was a problem hiding this comment.
why wouldn't we export it? It might be useful in other contexts and costs us nothing to export.
There was a problem hiding this comment.
It's not used at the moment and bypasses the current way we're recommending component tokens to be tested via themed.
| * @param token - the token as a CSS variable | ||
| * @returns string - the new value for the token | ||
| */ | ||
| export function assignTestTokenThemeValues(token: string): string { |
There was a problem hiding this comment.
Can we move this into commonTests? #9056 should help split up helpers by test topic.
Related Issue: #7180
Summary
Our chromium tests do a good job of ensuring the component token updates to styles do not break the existing UI but custom style application of tokens should be considered a data change, not a style change and therefor should be tested with Unit tests. These E2E tests are faster, less resource heavy, and less prone to human error.
However, E2E token tests are harder to set up. To facilitate ease-of-use for other developers this PR introduces a new commonTest utility called themed. This utility expects a calcite-component followed by an object of component token names as keys and each value being an object with a selector, shadowSelector, targetProp, and optional state. The state key will accept a string or an object for more selector specificity which is required when testing a "press"/"active" state.