fix: Update Test Selector types and improve functionality #2333
Conversation
| export type ByRoleMethods = | ||
| | "getByRole" | ||
| // | "getAllByRole" | ||
| | "queryByRole"; | ||
| // | "queryAllByRole" | ||
| // | "findByRole" | ||
| // | "findAllByRole" | ||
|
|
||
| export type ByTextMethods = | ||
| | "getByLabelText" | ||
| // | "getAllByLabelText" | ||
| | "queryByLabelText" | ||
| // | "queryAllByLabelText" | ||
| // | "findByLabelText" | ||
| // | "findAllByLabelText" | ||
| | "getByPlaceholderText" | ||
| // | "getAllByPlaceholderText" | ||
| | "queryByPlaceholderText" | ||
| // | "queryAllByPlaceholderText" | ||
| // | "findByPlaceholderText" | ||
| // | "findAllByPlaceholderText" | ||
| | "getByText" | ||
| // | "getAllByText" | ||
| | "queryByText" | ||
| // | "queryAllByText" | ||
| // | "findByText" | ||
| // | "findAllByText" | ||
| | "getByAltText" | ||
| // | "getAllByAltText" | ||
| | "queryByAltText" | ||
| // | "queryAllByAltText" | ||
| // | "findByAltText" | ||
| // | "findAllByAltText" | ||
| | "getByTitle" | ||
| // | "getAllByTitle" | ||
| | "queryByTitle"; | ||
| // | "queryAllByTitle" | ||
| // | "findByTitle" | ||
| // | "findAllByTitle" |
There was a problem hiding this comment.
Next PR, I plan to uncomment more of these.
|
|
||
| export const isRegExpString = (string: string) => /^\/*(.+)\/$/.test(string); | ||
|
|
||
| export const interpolateString = ( |
There was a problem hiding this comment.
Could you add some comments here clarifying what this should be used for? What risks if any, are we exposing ourselves to with using eval like this?
There was a problem hiding this comment.
I actually removed the functionality where this would be used, and I'm not sure I need to add it back at this point.
I'll put a note in here. The eval is safe because it only evals what someone passes to Odyssey from their unit test.
I might remove this function entirely in a future, so I marked it as @deprecated.
|
|
||
| this.name = "ElementError"; | ||
|
|
||
| console.error(element); |
There was a problem hiding this comment.
Logging the error in the constructor seems a bit strange, it might end up producing the log somewhere unexpected if the Error is created and then caught somewhere without immediately throwing. Probably doesn't matter much though.
There was a problem hiding this comment.
I didn't know a better way of doing this. When you throw an error, there's no way to pass an object along with the error; it always gets stringified, so I just logged it when the error came up.
Unless I'm wrong, I don't know if there's a method on the error like thrown() {} where I can log the HTML element.
I guess I could put the error in there:
| console.error(element); | |
| console.error("ElementError", element); |
| keyof LocalFeatureTestSelector["selector"]["options"], | ||
| string | RegExp | ||
| > | ||
| : Record<string, string>; |
There was a problem hiding this comment.
Is this an expected case? Or should this be a never?
There was a problem hiding this comment.
This type is actually unused now. But yes, it should've been never.
| role: infer Role; | ||
| }; | ||
| } | ||
| ? Role extends AriaRole[] |
There was a problem hiding this comment.
the formatting on these conditionals is not great. If possible I'd like to see these extracted into named types where the branches in the types are more understandable
There was a problem hiding this comment.
I agree, Prettier does a number on these types. They looked somewhat easier to read originally.
Can you provide an example of what you mean? I had these separated out before, but I didn't end up using the other types anywhere.
| role: Role[number]; | ||
| } | ||
| : object | ||
| : object) & |
There was a problem hiding this comment.
Any way we can use something more specific than object here?
There was a problem hiding this comment.
I would've preferred {}, but Odyssey's ESLint rules don't like it, and I dislike using // eslint-disable-next-line @typescript-eslint/ban-types. {} is the correct type though.
What I'm saying is, either this object has { options: something } or it doesn't have anything. It's essentially the same as never, but never & { queryMethod: QueryMethod } makes never.
| try { | ||
| capturedElement = getControlledElement({ element: containerElement }); | ||
| } catch (error) { | ||
| if (queryMethod === "query") { |
There was a problem hiding this comment.
Whats this branch about? If we're throwing immediately after it, why does it matter what this local variable is set to?
There was a problem hiding this comment.
Great catch! I meant to remove this code. It was something I was testing before I figured out the way to fix the HTMLElement | null issue.
| ); | ||
| } | ||
|
|
||
| if (!("accessibleText" in featureTestSelector)) { |
There was a problem hiding this comment.
This could be managed by the type system if we change the extends at the top of this function to AccessibleLabelSelector & TestSelector. Should user's be allowed to give a selector that doesn't have this already? Or is it expected that they should handle an error like this at runtime and recover from it?
There was a problem hiding this comment.
TestSelector isn't the only type that returns an element. It used to be, but now the FeatureSelector could have isControlledElement, and that also could follow you to an element.
You shouldn't be able to call this function if your type isn't valid, but the function itself doesn't know that. I tried using a ternary and null if it didn't have accessible text, but then I had to use ?.getAccessibleText because TypeScript could never be sure the function existed. It's also a runtime check for capturedElement as it could be null if you use "query" as the queryMethod.
A few things about that:
- It doesn't make sense that
isControlledElementisn't anotherselectorvariant since it's related to element selection.isControlledElementused to be part of the feature itself rather than aFeatureTestSelectorbecause you can't have a controlled element that isn't a child of another element. Also, does it make sense that a child element is a feature or that it's an associated element? - I should rename
TestSelectortoElementSelectoror something else. It was calledTestSelectorbecause it was aTestingLibrarySelectororiginally andFeatureSelectorwas a way to get features of the component.TestSelectorshould be more like the whole type rather thanFeatureTestSelector. I'm gonna play around with names. - The
isControlledElementcheck throws errors, and it should only throw when it'sgetrather thanquery. I have a// TODO, but I never yet fixed it in this branch.
| const getAccessibleText = < | ||
| LabelName extends LocalFeatureTestSelector extends AccessibleLabelSelector | ||
| ? keyof LocalFeatureTestSelector["accessibleText"] | ||
| : keyof AccessibleLabelSelector, |
There was a problem hiding this comment.
Is this conditional correct? keyof AccessibleLabelSelector would only ever be accessibleText right? Where as the branch above it would be whatever string is in that record. Is accessibleText an expected labelName here?
There was a problem hiding this comment.
I changed it to never for the situation you don't have accessibleText defined on your object.
| >( | ||
| labelName: LabelName, | ||
| ) => { | ||
| if (!capturedElement) { |
There was a problem hiding this comment.
Unfortunate that typescript can't handle these inferences in closures like this. I think 5.5 might fix it? Not sure though.
There was a problem hiding this comment.
We're on TypeScript 5.5.4.
I originally had this:
FeatureName extends keyof (typeof testSelectors)["Feature"]But it only worked because at the time, I verified feature was in testSelectors before creating the function. It also said the value of .feature was unknown.
Not sure how I'd narrow it down in this spot. If you know a way, I'd love to narrow it down further!
| ? LocalFeatureTestSelector["feature"][FeatureName] extends TestSelector | ||
| ? keyof LocalFeatureTestSelector["feature"][FeatureName]["selector"]["options"] | ||
| : string | ||
| : string, |
There was a problem hiding this comment.
Are these string cases expected? Or should they be nevers?
There was a problem hiding this comment.
Good question.
InnerQuerySelectorProps doesn't return { options: something } if you don't have options, so string or never, it doesn't matter. Regardless, I changed it to never since that's more accurate 👍.
| : FeatureTestSelector, | ||
| )( | ||
| // @ts-expect-error: Type '{ role?: AriaRole | undefined; options?: Record<LocalFeatureTestSelector extends FeatureSelector ? LocalFeatureTestSelector["feature"][FeatureName] extends TestSelector ? keyof LocalFeatureTestSelector["feature"][FeatureName]["selector"]["options"] : string : string, string | RegExp> | undefined; element: HTMLElement...' is not assignable to type '(LocalFeatureTestSelector extends FeatureSelector ? LocalFeatureTestSelector["feature"][FeatureName] : FeatureTestSelector) extends { ...; } ? Role extends AriaRole[] ? { ...; } : object : object'.ts(2345) | ||
| // `as featureTestSelector.feature[featureName]` narrows the props down enough that TypeScript errors here. We're passing the correct information, but it doesn't know that, and it's difficult to fix this. -Kevin Ghadyani |
There was a problem hiding this comment.
Would be interested in some more info on what the exact mismatch between the expected types is here. What is the part that doesn't line up? Obviously theres something to do with narrowing the feature[FeatureName] type, but it seems like theres a fair amount of info being omitted from the error message
There was a problem hiding this comment.
I think it's something to do with options specifically. But even if I uncomment these things, it errors.
If you wanna take a look, I'd gladly accept.
Changing this:
querySelector(
featureTestSelector.feature[
childProps.featureName
] as LocalFeatureTestSelector extends FeatureSelector
? LocalFeatureTestSelector["feature"][FeatureName]
: FeatureTestSelector,
)Into this, then it works:
querySelector(
featureTestSelector.feature[
childProps.featureName
],
)But the types don't show correctly for deeply-nested features. FeatureName becomes string or unknown.
The proper way to handle this is to simply return querySelector(testSelectors, element)(featureName), but then you have to call it like:
selectChild("listItem")({
options: {
label: /Destination/
}
})I had this before but didn't think the currying made sense to people. The API was super wonky to use. I wish I had an easier way of accepting both a feature name and a set of optional options.
| return { | ||
| element: capturedElement as LocalQueryMethod extends "get" | ||
| ? HTMLElement | ||
| : HTMLElement | null, |
There was a problem hiding this comment.
get here doesn't have a nullable case, because it just throws in the situation where it can't find the element right? meanwhile find/query will just return a null possibly. For the case of query, would it ever return multiple elements, or is it always a single one like this?
There was a problem hiding this comment.
Only single. queryAll does multiple, but that's currently not supported.
KevinGhadyani-Okta
changed the title
KevinGhadyani-Okta
changed the title
KevinGhadyani-Okta
changed the title
…gerous it could be
…moved in a future PR
KevinGhadyani-Okta
force-pushed
the
OKTA-794668-test-selectors-functionality
branch
from
34fa3da to
f9eb8e3
Compare
KevinGhadyani-Okta
force-pushed
the
OKTA-794668-test-selectors-functionality
branch
from
f9eb8e3 to
b5b4289
Compare
|
This PR is currently waiting on @adapalmer-okta to be back in the office and see if there are any further changes to be made. |
oktapp-aperture-okta
bot
deleted the
OKTA-794668-test-selectors-functionality
branch
OKTA-794668 fix: selecting controlled elements feat: adds ability to have type-safe roles in queries fix: updates Callout test selectors test test 2 test 3 test 4 test 5 fix: reconfigured querySelector with correct types fix: improved test selectors API fix: adds typing for getAccessibleText fix: cleanup test selectors code fix: removes rxjs fix: prettify code fix: fixed test error in Callout stories fix: removes explicit odysseyTestSelectors file fix: updates ElementError logging with message denoting ElementError fix: defaults `ChildQueryMethod` as `"get"` to match other instances fix: minor fix to change Options key to never where it wouldn't exist anyway fix: adds comment around `interpolateString` to make it clear how dangerous it could be fix: marks interpolateString as @deprecated as it will probably be removed in a future PR fix: removes unused type in querySelector fix: massively renamed TestSelector types to be easily understood fix: moves element selection outside of querySelector fix: adds missing accessibleText to Select fix: renames testSelectors to testSelector in all supported components Merge branch 'main' into OKTA-794668-test-selectors-functionality fix: fixes small type issues and Autocomplete merge issue fix: fixes broken Select interaction test
OKTA-794668
Summary
FeatureTestSelectortypes.getAccessibleTextfor accessibility testing in Java.Testing & Screenshots
Tests coming in a future PR. Since none of this code is used yet, I'm still working through it. Next PR will be adding separate Playwright and TypeScript tests.