Skip to content
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

[🐛 Bug]: Typing issues with Jasmine integration #11533

Closed
3 tasks done
BenoitZugmeyer opened this issue Oct 25, 2023 · 10 comments · Fixed by #11635
Closed
3 tasks done

[🐛 Bug]: Typing issues with Jasmine integration #11533

BenoitZugmeyer opened this issue Oct 25, 2023 · 10 comments · Fixed by #11635
Labels
Bug 🐛 help wanted Issues that are free to take by anyone interested

Comments

@BenoitZugmeyer
Copy link

Have you read the Contributing Guidelines on issues?

WebdriverIO Version

8.20.5

Node.js Version

18.18.2

Mode

WDIO Testrunner

Which capabilities are you using?

No response

What happened?

In our project we are using the WDIO testrunner with TypeScript and Jasmine integration. To use the correct TS typings, we include types in a specific order, so the types from @wdio/global/types are partially overriden by the jasmine types, especially for the expect global variable.

When upgrading from WDIO 8.20.0 to 8.20.5, our test suite fails with typing errors because the expect global variable has the Jest expect type, not the Jasmine one. I believe this is because of PR #11524, which makes the wdio global types take precedence over jasmine types.

What is your expected behavior?

I would expect to be able to use the correct jasmine types.

Would it be possible to revert the way it was before 8.20.5? Else, would it be possible to have documentation on how to setup the Jasmine integration with TypeScript (the current documentation does not mention types)?

How to reproduce the bug.

Relevant log output

test/specs/form.spec.ts:26:22 - error TS2339: Property 'toBeTrue' does not exist on type 'Matchers<void | Promise<void>, boolean>'.

26         expect(true).toBeTrue();
                        ~~~~~~~~


Found 1 error in test/specs/form.spec.ts:26

Code of Conduct

  • I agree to follow this project's Code of Conduct

Is there an existing issue for this?

  • I have searched the existing issues
@BenoitZugmeyer BenoitZugmeyer added Bug 🐛 Needs Triaging ⏳ No one has looked into the issue yet labels Oct 25, 2023
@christian-bromann
Copy link
Member

Thanks for reporting!

I think this is a bug we should view separately and find a solution for it. Maybe as a workaround for now you can cast the type to match it the jasmine expect.

We greatly appreciate any contributions that help resolve the bug. While we understand that active contributors have their own priorities, we kindly request your assistance if you rely on this bug being fixed. We encourage you to take a look at our contribution guidelines or join our friendly Discord development server, where you can ask any questions you may have. Thank you for your support, and cheers!

@erwinheitzman
Copy link
Member

erwinheitzman commented Oct 26, 2023

Like Christian said this is indeed a larger issue that we need to tackle, that said, you want to prevent using synchronous assertions as they are inherently flaky. You can replace it with this:

await expect(true).toBeTruthy();

Always use webdriverio assertions, for more information as to why, checkout our best practices guide on thic topic.

@christian-bromann christian-bromann added help wanted Issues that are free to take by anyone interested and removed Needs Triaging ⏳ No one has looked into the issue yet labels Oct 26, 2023
@BenoitZugmeyer
Copy link
Author

I'm a bit confused: what is the point of the Jasmine integration if we cannot use Jasmine assertions?

@erwinheitzman
Copy link
Member

I'm a bit confused: what is the point of the Jasmine integration if we cannot use Jasmine assertions?

The point of Jasmine, Mocha and Cucumber is that they are test runners. They are however unit test frameworks by nature and synchronous assertions are prone to failure when ran in an environment in which all kinds of delays can happen (render cycle, network traffic, JavaScript, slow system, etc), that's why you should use the asynchronous assertions instead.

@christian-bromann
Copy link
Member

@erwinheitzman I think this is more about typing issues rather than which assertions to use. IMHO using a synchronous assertion like expect(await $(elem).getValue()).toBe('foobar') works as well as any other assertion.

@erwinheitzman
Copy link
Member

erwinheitzman commented Oct 29, 2023

@erwinheitzman I think this is more about typing issues rather than which assertions to use. IMHO using a synchronous assertion like expect(await $(elem).getValue()).toBe('foobar') works as well as any other assertion.

I agree that the types should work but the issue is really easy to work around using asynchronous assertions and we have listed them as a best practice for a reason. Perhaps I am too harsh here but I ban synchronous assertions in any of the projects I work on because it is simply a bad idea to use them if you want flaky free tests.

I definitely want to keep this issue open and want to have the types fixed at some point.

Any contributions to this are greatly appreciated

@BenoitZugmeyer
Copy link
Author

really easy to work around using asynchronous assertions and we have listed them as a best practice for a reason

This issue is not related to async vs sync assertions, but more about using wdio default expect function vs the jasmine one. This has been an issue for us in the past (see #8126).

If using the expect provided by wdio instead of the Jasmine expect function is your official recommendation, I think you should at least mention it in the Using jasmine guide.

Any contributions to this are greatly appreciated

I would be happy to create a PR reverting #11524, but I don't think this is what you want :)

@christian-bromann
Copy link
Member

@BenoitZugmeyer we should resolve this by making sure the Jasmine types are properly propagated for Jasmine assertions in https://github.com/webdriverio/expect-webdriverio. We already have everything in there since we suppose to support this but it seems to fail now with the change you mention above.

@erwinheitzman
Copy link
Member

erwinheitzman commented Oct 30, 2023

really easy to work around using asynchronous assertions and we have listed them as a best practice for a reason

This issue is not related to async vs sync assertions, but more about using wdio default expect function vs the jasmine one. This has been an issue for us in the past (see #8126).

If using the expect provided by wdio instead of the Jasmine expect function is your official recommendation, I think you should at least mention it in the Using jasmine guide.

Any contributions to this are greatly appreciated

I would be happy to create a PR reverting #11524, but I don't think this is what you want :)

I totally agree with you that we should add it in our docs (not sure if the page you linked is the place to put it but we definitely need to mention it somewhere imo), especially because when you import our globals, you import the wdio expect types.

Example:

import { expect } from '@wdio/globals';

describe("example", () => {
  it("does not work", async () => {
    expect(true).toBeTrue()
  });

  it("works", async () => {
    await expect(true).toBeTruthy()
  });
});

Might be worth putting it in the Typescript docs as a similar issue can occur when you add jasmine as a type instead of expect-webdriverio which I have also seen before in which case it will not see the wdio methods like toBeThruthy.

@christian-bromann
Copy link
Member

To be clear: we should and do support Jasmines assertions since this is the whole point of integrating with the Jasmine framework. Here is some of the code that makes this happen: https://github.com/webdriverio/webdriverio/blob/main/packages/wdio-jasmine-framework/src/index.ts#L407-L434

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug 🐛 help wanted Issues that are free to take by anyone interested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants