-
-
Notifications
You must be signed in to change notification settings - Fork 59
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:
awaitfor 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 theexpectLib, 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
neverto 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
expectInspired by this comment, we aim to align our approach more closely with the
expectlibrary, potentially reducing some inconsistencies.Pros:
closeToandarrayOfworking out-of-the-box withjest-matcher-utilsnotis now handled automatically, as it was previously overwritten.Cons:
expectlibrary changes, but less likely to happen since it is a solid base shared in multiple places.Jasmine
Jasmine is special because
expectAsyncalways returns a Promise as the final result, unlike other frameworks. Moreover, inexpect-webriveriowe augment the Jasmine namespace to have wdio matchers onexpectAsync; however, in wdio-jasmine-framework, we force theexpectto 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/jestand@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
toMatchSnapshotandtoMatchInlineSnapshotconflict with those in Jest's library. Best effort was made to support both wdio and Jest definitions.We cannot only use
@jest/globaland augment it; we need@types/jest. This Jest's issue might changes that one dayOther:
browser,elementandmockmatchers #1408; however, this PR proposes an alternative solution that addresses them.wdio/await-expectdoes not interfere with this PR's changes. Moreover, this plugin could be reviewed to utilizeno-floating-promisesand perform even better once this PR is merged.toBeortoEqual, making it less interesting. This code is the problem; supporting only custom matchersawaithere, creating another problem if point 1 is resolved. With basic matchers, noawaitshould be requiredBREAKING
standalone.d.tsis deleted since the default ExpectWebDriverIO namespace is now correctly working and covering the standalone casejest-global.d.tsis renamed toexpect-global.d.tssince it is unrelated to Jest; we provideexpectglobally for the WDIO'sexpect.samplewas onExpectWebdriverIO.PartialMatcher, now it is onWdioAsymmetricMatcherexpect.not.anything()/any(X)typing is no longer supported since in Jest it was not even supportedvoid | Promise<void>, making it "working" with the forcedexpectAsyncneeds to be awaited, but now it isvoid"expect-webdriverio/expect-wdio-jasmine-async"in their tsconfig.josn#typesexpect.notnow has a different type for AsymetricsMatcher vs InverseAsymmetricMatchers since 'any' and 'anything' are not supported in inverse, as theexpectand 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/jasmineandexpectAsyncTasks
wdio/await-expectcould counter changes from this PRIntegrate in webdriverio with yalcyalc is not workingexpectis the right approachPromise<void>for basic matcher under Jasmine because of the forcedexpectAsyncwhile not breaking Jest and standalone!Understand why we need to- Still not sure, butexpect.notto register asymmetrics matchers here since it is no longer exposedInverseAsymmetricMatcherstype is now provided to better support different AsymmetricMatchers and Inverse AsymmetricMatchers keyof for webdriver project