-
-
Notifications
You must be signed in to change notification settings - Fork 58
Enhanced expect wdio typing #1863
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
base: main
Are you sure you want to change the base?
Enhanced expect wdio typing #1863
Conversation
afe365c
to
8a8724c
Compare
bcfe508
to
3aaaac3
Compare
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.
Amazing work 🙏 ❤️ some minor nits
ToBe seems fine to not return a promise since I did not find example of forcing to be promises when using element or chainable Code review
@christian-bromann I would be ready for a release + integration in WebdriverIO Two "important" points:
In const matcherKey = SUPPORTED_ASYMMETRIC_MATCHER[arg.$$typeof as keyof typeof SUPPORTED_ASYMMETRIC_MATCHER] as keyof AsymmetricMatchers
const inverseMatcherKey = SUPPORTED_ASYMMETRIC_MATCHER[arg.$$typeof as keyof typeof SUPPORTED_ASYMMETRIC_MATCHER] as keyof InverseAsymmetricMatchers
const matcher = ('inverse' in arg && arg.inverse ? expect.not[inverseMatcherKey] : expect[matcherKey]) as unknown as (sample: string) => unknown |
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 👍 exceptional work!
(some minor formatting which I will go ahead and merge)
I will merge and update WebdriverIO core tomorrow. |
Temporary alternative: |
@christian-bromann @erwinheitzman I thought this PR was ready to merge, but I feel there is still work to be done to get your whole agreement. |
@dprevost-LMI hi I think we are good to go unless Christian is waiting for something (referring to the merging of the core) |
TL;DR
Fix typing to:
await
for strings, allow correct usage of no-floating-promises)Summary
1. Wrong return types
Have promise used only when required so that no-floating-promises can be used and detect when await is missing throughout the code, including the wdio expect cases.
In short, even though it was more complicated, instead of having both
void | Promise<void>
returned by expect/matchers, we are now returningPromise<void>
only for Wdio Matchers and for the default ones from theexpect
Lib, it isvoid
.2. TS Error when using element/browser matcher with the wrong type
Have a Typescript error when using the wrong actual type
Using an approach where we define a function based on the typing of the actual T, we can make TS use
never
to error cases where the type is undesiredvariable
.For
toHaveHeight
, we had to combine the two signatures in one! This point could need a debate!Now extending
expect
Inspired by this comment, we aim to align our approach more closely with the
expect
library, potentially reducing some inconsistencies.Pros:
closeTo
andarrayOf
working out-of-the-box withjest-matcher-utils
not
is now handled automatically, as it was previously overwritten.Cons:
expect
library changes, but less likely to happen since it is a solid base shared in multiple places.Jasmine
Jasmine is special because
expectAsync
always returns a Promise as the final result, unlike other frameworks. Moreover, inexpect-webriverio
we augment the Jasmine namespace to have wdio matchers onexpectAsync
; however, in wdio-jasmine-framework, we force theexpect
to beexpectAsync
.This raises several questions and inconsistencies that are outside the scope of this PR. This issue was raised to document what needs to happen to help clarify Jasmine support later.
In the context of this PR, we have documented what is supported and some limitations, and ensured, through tests, that these are still supported.
Jest
Even though we support
@types/jest
and@jest/global
, the latter is recommended since Jest's maintainer does not support/recommend@types/jest
, and it may even be behind and therefore causing problems (subject to be dropped later)The matchers
toMatchSnapshot
andtoMatchInlineSnapshot
conflict with those in Jest's library. Best effort was made to support both wdio and Jest definitions.We cannot only use
@jest/global
and augment it; we need@types/jest
. This Jest's issue might changes that one dayOther:
browser
,element
andmock
matchers #1408; however, this PR proposes an alternative solution that addresses them.wdio/await-expect
does not interfere with this PR's changes. Moreover, this plugin could be reviewed to utilizeno-floating-promises
and perform even better once this PR is merged.toBe
ortoEqual
, making it less interesting. This code is the problem; supporting only custom matchersawait
here, creating another problem if point 1 is resolved. With basic matchers, noawait
should be requiredBREAKING
standalone.d.ts
is deleted since the default ExpectWebDriverIO namespace is now correctly working and covering the standalone casejest-global.d.ts
is renamed toexpect-global.d.ts
since it is unrelated to Jest; we provideexpect
globally for the WDIO'sexpect
.sample
was onExpectWebdriverIO.PartialMatcher
, now it is onWdioAsymmetricMatcher
expect.not.anything()/any(X)
typing is no longer supported since in Jest it was not even supportedvoid | Promise<void>
, making it "working" with the forcedexpectAsync
needs to be awaited, but now it isvoid
"expect-webdriverio/expect-wdio-jasmine-async"
in their tsconfig.josn#typesexpect.not
now has a different type for AsymetricsMatcher vs InverseAsymmetricMatchers since 'any' and 'anything' are not supported in inverse, as theexpect
and Jest library does.Testing
Jest:
@jest/global
+ no-floating-promise@jest/global
+@types/jest
+ no-floating-promiseJasmine:
@types/Jasmine
+@wdio/jasmine-framework
+ no-floating-promiseexpect-webdriverio/jasmine
andexpectAsync
Tasks
wdio/await-expect
could counter changes from this PRIntegrate in webdriverio with yalcyalc is not workingexpect
is the right approachPromise<void>
for basic matcher under Jasmine because of the forcedexpectAsync
while not breaking Jest and standalone!Understand why we need to- Still not sure, butexpect.not
to register asymmetrics matchers here since it is no longer exposedInverseAsymmetricMatchers
type is now provided to better support different AsymmetricMatchers and Inverse AsymmetricMatchers keyof for webdriver project