fix: Refactoring and fixes prior to support array selector $$#2126
fix: Refactoring and fixes prior to support array selector $$#2126dprevost-LMI wants to merge 10 commits into
Conversation
Greptile SummaryThis PR refactors and cleans up the matcher internals as a preparatory step toward supporting array selectors (
Confidence Score: 4/5Safe to merge after addressing the null-guard removal in The removal of
|
| Filename | Overview |
|---|---|
| src/util/refetchElements.ts | Removed elements.parent && null guard before elements.foundWith in elements.parent; isElementArrayOrChainable only checks property existence, not that the value is non-null, so a null parent would throw TypeError at runtime. |
| src/matchers/element/toHaveAttribute.ts | Refactored into proper overloads with isStringOptions dispatch; deprecated overload is listed first, shadowing the new cleaner signatures in TypeScript resolution. |
| src/util/commandOptionsUtils.ts | New isStringOptions type guard uses some to check allowed keys, accepting objects that mix valid option keys with unrelated properties. |
| src/util/waitUntil.ts | Extracted waitUntil from src/utils.ts to its own module; logic is unchanged, now exported and well-tested. |
| src/util/numberOptionsUtil.ts | Adds NumberMatcher class and validateNumberOptions helper; validateNumberOptions is used in toBeElementsArrayOfSize, while NumberMatcher/numberMatcherTester are preparatory work flagged in a previous thread. |
| src/util/elementsUtil.ts | Replaces isElementArray with richer type guards: isElementArrayOrChainable, isStrictlyAwaitedElementArray, isElement, isElementArrayLike, isElementOrArrayLike, isElementOrNotEmptyElementArray. |
| src/util/formatMessage.ts | Fixes numberError to use isDefined instead of truthiness checks, correctly handling 0 boundary values; adds array-of-elements branch to getSelectors. |
| src/matchers/element/toHaveElementClass.ts | Renamed from toHaveClass.ts; old toHaveClass and toHaveClassContaining are preserved as deprecated wrappers. |
| src/matchers/element/toBeSelected.ts | toBeChecked now delegates to toBeSelected via this.matcherName, eliminating the double-fire of beforeAssertion/afterAssertion that existed in the old code. |
| src/matchers/elements/toBeElementsArrayOfSize.ts | Uses new validateNumberOptions helper; merges numberCommandOptions into commandOptions for proper wait/interval propagation. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["toHaveAttribute(el, attr, valueOrOptions?, opts?)"] --> B{isStringOptions\nvalueOrOptions?}
B -- yes --> C["toHaveAttributeFn\n(presence check only)"]
B -- no --> D["toHaveAttributeAndValue\n(value comparison)"]
C --> E["waitUntil → conditionAttributeIsPresent\nel.getAttribute(attr) !== null"]
D --> F["waitUntil → conditionAttributeValueMatchWithExpected\ncompareText(attr, expectedValue, opts)"]
E --> G[enhanceError → AssertionResult]
F --> G
subgraph elementsUtil
H[isElementArrayOrChainable] --> I{Array.isArray\n+ selector + parent + foundWith}
J[isStrictlyAwaitedElementArray] --> K{isElementArrayOrChainable\n+ getElements}
L[isElement] --> M{object + selector\n+ parent + getElement}
N[isElementArrayLike] --> O{isElementArrayOrChainable\nOR every isElement}
end
subgraph refetchElements
P["refetchElements(elements, wait)"] --> Q{elements &&\nwait > 0 &&\nisElementArrayOrChainable}
Q -- yes --> R["elements.foundWith in elements.parent\n⚠️ parent not null-checked"]
R --> S["browser.$$(selector) re-fetch"]
end
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A["toHaveAttribute(el, attr, valueOrOptions?, opts?)"] --> B{isStringOptions\nvalueOrOptions?}
B -- yes --> C["toHaveAttributeFn\n(presence check only)"]
B -- no --> D["toHaveAttributeAndValue\n(value comparison)"]
C --> E["waitUntil → conditionAttributeIsPresent\nel.getAttribute(attr) !== null"]
D --> F["waitUntil → conditionAttributeValueMatchWithExpected\ncompareText(attr, expectedValue, opts)"]
E --> G[enhanceError → AssertionResult]
F --> G
subgraph elementsUtil
H[isElementArrayOrChainable] --> I{Array.isArray\n+ selector + parent + foundWith}
J[isStrictlyAwaitedElementArray] --> K{isElementArrayOrChainable\n+ getElements}
L[isElement] --> M{object + selector\n+ parent + getElement}
N[isElementArrayLike] --> O{isElementArrayOrChainable\nOR every isElement}
end
subgraph refetchElements
P["refetchElements(elements, wait)"] --> Q{elements &&\nwait > 0 &&\nisElementArrayOrChainable}
Q -- yes --> R["elements.foundWith in elements.parent\n⚠️ parent not null-checked"]
R --> S["browser.$$(selector) re-fetch"]
end
Reviews (2): Last reviewed commit: "Fix parentheses precedence" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| export async function toHaveElementProperty( | ||
| received: WdioElementMaybePromise, | ||
| property: string, | ||
| value?: string | RegExp | AsymmetricMatcher<string> | null, | ||
| expected: string | number | null | RegExp | AsymmetricMatcher<string>, | ||
| options: ExpectWebdriverIO.StringOptions = DEFAULT_OPTIONS | ||
| ) { | ||
| const isNot = this.isNot | ||
| const { expectation = 'property', verb = 'have' } = this | ||
| const matcherName = 'toHaveElementProperty' | ||
|
|
||
| await options.beforeAssertion?.({ | ||
| matcherName: 'toHaveElementProperty', | ||
| expectedValue: [property, value], | ||
| matcherName, | ||
| expectedValue: [property, expected], | ||
| options, | ||
| }) | ||
|
|
||
| let el = await received?.getElement() | ||
| let prop: unknown | ||
| const pass = await waitUntil( | ||
| async () => { | ||
| const result = await executeCommand.call(this, el, condition, options, [property, value]) | ||
| const result = await executeCommand.call(this, el, condition, options, [property, expected]) | ||
| el = result.el as WebdriverIO.Element | ||
| prop = result.values | ||
|
|
||
| return result.success | ||
| }, | ||
| isNot, | ||
| options | ||
| ) | ||
|
|
||
| let message: string | ||
| if (value === undefined) { | ||
| if (expected === undefined) { | ||
| message = enhanceError(el, !isNot, pass, this, verb, expectation, property, options) | ||
| } else { | ||
| const expected = wrapExpectedWithArray(el, prop, value) | ||
| message = enhanceError(el, expected, prop, this, verb, expectation, property, options) | ||
| const wrappedExpected = wrapExpectedWithArray(el, prop, expected) | ||
| message = enhanceError(el, wrappedExpected, prop, this, verb, expectation, property, options) | ||
| } |
There was a problem hiding this comment.
expected is now required, removing the property-existence check
The parameter was changed from optional value?: string | RegExp | AsymmetricMatcher<string> | null to required expected: string | number | null | RegExp | AsymmetricMatcher<string>. Any existing call of el.toHaveElementProperty('prop') (used to verify property existence) is now a TypeScript error. The implementation still contains if (expected === undefined) on line 70, which is now dead code since TypeScript prevents passing undefined. If callers must now pass null to check existence, that is a silent behavioral inversion: the old condition returned { result: true } when value was undefined, while the new condition returns { result: true } only when value === null.
There was a problem hiding this comment.
Yes, to review later, we should allow an overloaded function to check the existing props and be able to pass command options without needing undefined
So something like
toHaveElementProperty('myPropName', {wait: 1})
It was achieved with toHaveAttribute so it's doable too here
There was a problem hiding this comment.
Good catch. toHaveAttribute is the perfect pattern to follow here. The key is splitting into two separate internal functions and dispatching based on whether expected was provided. Here's what the refactor should look like for toHaveElementProperty:
// Existence-only check (no value)
async function conditionProperty(el: WebdriverIO.Element, property: string) {
const prop = await el.getProperty(property)
if (prop === null || prop === undefined) {
return { result: false, value: prop }
}
return { result: true, value: prop }
}
// Value comparison check
async function conditionPropertyAndValue(
el: WebdriverIO.Element,
property: string,
value: string | number | RegExp | AsymmetricMatcher<string>,
options: ExpectWebdriverIO.StringOptions
) {
// existing condition logic, minus the value === null branch
...
}
export async function toHaveElementProperty(
received: WdioElementMaybePromise,
property: string,
expected?: string | number | null | RegExp | AsymmetricMatcher<string>,
options: ExpectWebdriverIO.StringOptions = DEFAULT_OPTIONS
) {
// ...beforeAssertion...
const result = expected !== undefined
? await toHaveElementPropertyAndValue.call(this, received, property, expected, options)
: await toHaveElementPropertyFn.call(this, received, property)
// ...afterAssertion...
return result
}This mirrors toHaveAttribute exactly and means toHaveElementProperty('myPropName', { wait: 1 }) won't naturally work unless you add explicit overload signatures:
export function toHaveElementProperty(received: WdioElementMaybePromise, property: string, options?: ExpectWebdriverIO.StringOptions): Promise<...>
export function toHaveElementProperty(received: WdioElementMaybePromise, property: string, expected: string | number | null | RegExp | AsymmetricMatcher<string>, options?: ExpectWebdriverIO.StringOptions): Promise<...>That way toHaveElementProperty('myPropName', { wait: 1 }) works naturally without needing to pass undefined. Worth filing this as a follow-up issue to track it separately from this PR.
| if (typeof attributeValue !== 'string') { | ||
| return { result: false, value: attributeValue } | ||
| } | ||
|
|
There was a problem hiding this comment.
toHaveAttributeAndValue exported without a default for options
The function is exported (used by toHaveHref, toHaveId, and toHaveClassContaining) but options has no default value. A direct call like toHaveAttributeAndValue(el, 'attr', 'val') will pass undefined as options, causing options.wait to throw at runtime. All current internal callers happen to pass options, but the exported surface is fragile.
There was a problem hiding this comment.
toHaveHref & toHaveId & toHaveClassContaining use default values in their signatures, so they pass default values, no?
Was the removal intended to ensure the defaults come from above, no?
Or maybe since we process only a subset, if we pass only stringOptions without commandOptions, the default still applies?
There was a problem hiding this comment.
You're right — my original concern was overstated. Looking at the actual callers:
toHaveHref:options: ExpectWebdriverIO.StringOptions = DEFAULT_OPTIONStoHaveId:options: ExpectWebdriverIO.StringOptions = DEFAULT_OPTIONStoHaveAttribute(the main entry point):options: ExpectWebdriverIO.StringOptions = DEFAULT_OPTIONS
All callers default options to DEFAULT_OPTIONS before passing it down, so toHaveAttributeAndValue always receives a fully-populated object — never undefined. The design is intentional: defaults are applied once at the top of the call chain, and inner functions are pure helpers that trust their callers.
My original comment was incorrect. Withdrawing it.
| export class NumberMatcher { | ||
| constructor(private options: ExpectWebdriverIO.NumberOptions = {}) {} | ||
|
|
||
| equals(actual: number | undefined): boolean { | ||
| if ( actual === undefined ) { | ||
| return false | ||
| } | ||
|
|
||
| if (isNumber(this.options.eq)) { | ||
| return actual === this.options.eq | ||
| } | ||
|
|
||
| if (isNumber(this.options.gte) && isNumber(this.options.lte)) { | ||
| return actual >= this.options.gte && actual <= this.options.lte | ||
| } | ||
|
|
||
| if (isNumber(this.options.gte)) { | ||
| return actual >= this.options.gte | ||
| } | ||
|
|
||
| if (isNumber(this.options.lte)) { | ||
| return actual <= this.options.lte | ||
| } | ||
|
|
||
| return false | ||
| } | ||
|
|
||
| toString(): string { | ||
| if (isNumber(this.options.eq)) { | ||
| return String(this.options.eq) | ||
| } | ||
|
|
||
| if (isDefined(this.options.gte) && isDefined(this.options.lte)) { | ||
| return `>= ${this.options.gte} && <= ${this.options.lte}` | ||
| } | ||
|
|
||
| if (isDefined(this.options.gte)) { | ||
| return `>= ${this.options.gte}` | ||
| } | ||
|
|
||
| if (isDefined(this.options.lte)) { | ||
| return `<= ${this.options.lte}` | ||
| } | ||
|
|
||
| return 'Incorrect number options provided' | ||
| } | ||
|
|
||
| toJSON(): string | number { | ||
| // Return the actual number for exact equality, so it serializes as 0 not "0" | ||
| if (isNumber(this.options.eq)) { | ||
| return this.options.eq | ||
| } | ||
| return this.toString() | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Custom tester for number matchers to be used by the equal of expect during failure message generation | ||
| */ | ||
| export const numberMatcherTester = (a: unknown, b: unknown): boolean | undefined => { | ||
| const isNumberMatcherA = a instanceof NumberMatcher | ||
| const isNumberMatcherB = b instanceof NumberMatcher | ||
|
|
||
| if (isNumberMatcherA && isNumber(b)) { | ||
| return a.equals(b) | ||
| } | ||
|
|
||
| if (isNumberMatcherB && isNumber(a)) { | ||
| return b.equals(a) | ||
| } | ||
|
|
||
| // Return undefined to let other testers handle it | ||
| return undefined | ||
| } |
There was a problem hiding this comment.
NumberMatcher and numberMatcherTester are defined but not integrated
NumberMatcher and numberMatcherTester are tested in test/util/numberOptionsUtil.test.ts but no production file in this PR imports or calls them. They appear to be preparatory work for a future feature. Adding unused exports to a published package increases bundle size and creates dead surface area; consider gating them behind a separate commit once they are wired up.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
TODO dpevost to review later
dprevost-LMI
force-pushed
the
prior-refactoring-for-$$-support
branch
from
95ebc65 to
fcfeca9
Compare
1 participant
No description provided.