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

Adding ability to get pseudo-elements css value via getCSSProperty #7709 #11570

Conversation

Pawel1894
Copy link
Contributor

Proposed changes

Added possibility to select pseudo-element css value. It is not defined in Webdriver spec so I did it via browser execute and did not modify protocols.

I didn't found a way to get browser element other than passing it as argument. I also did split getCSSProperty to smaller functions because it was hard to understand with all the comments and pseudoElement ifs.

Example usage:

element.getCSSProperty('width', {
   target: '::before',
   browser
})

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update

Checklist

  • I have read the CONTRIBUTING doc
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • I have added proper type definitions for new commands (if appropriate)

Further comments

I don't know how to mock window object with got.ts to be able to add tests for it in getCSSProperty.test.ts. I tried to use vitest spyOn to mock window object but it does not work.

Reviewers: @webdriverio/project-committers

Copy link
Member

@christian-bromann christian-bromann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to get unit tests for this.

@Pawel1894
Copy link
Contributor Author

It would be nice to get unit tests for this.

As I wrote in PR comment. I have tried to mock window object and make it work with got, but nothing worked, no idea how to do it.

@christian-bromann
Copy link
Member

I have tried to mock window object and make it work with got, but nothing worked, no idea how to do it.

There is no need to mock the window object. We already have some unit tests for the command which mocks the got package to fake requests to browser drivers (so the function within execute is never really being run). You can find the mock specification in ./__mocks__/got.ts.

@Pawel1894
Copy link
Contributor Author

I have tried to mock window object and make it work with got, but nothing worked, no idea how to do it.

There is no need to mock the window object. We already have some unit tests for the command which mocks the got package to fake requests to browser drivers (so the function within execute is never really being run). You can find the mock specification in ./__mocks__/got.ts.

Nice, I'll look into it on saturday 😄

@Pawel1894
Copy link
Contributor Author

I have tried to mock window object and make it work with got, but nothing worked, no idea how to do it.

There is no need to mock the window object. We already have some unit tests for the command which mocks the got package to fake requests to browser drivers (so the function within execute is never really being run). You can find the mock specification in ./__mocks__/got.ts.

Nice, I'll look into it on saturday 😄

Ok, so I tried to make test work , but without success.

When I use window.getComputedStyle inside execute I'm getting this error:

ReferenceError: window is not defined

eval __mocks__/got.ts:207:24
    205|     case `/session/${sessionId}/execute/sync`: {
    206|         // mock execute
    207|         const script = Function(params.json.script)
       |                        ^
    208|         const args = params.json.args.map((arg: any) => (arg && (

I tried to look for examples, I used some other window properties like window.scrollY inside my function and it worked.. but this one does not. Do I need to also add this specific function somewhere in mocks to make it work?

@christian-bromann
Copy link
Member

@Pawel1894 for this test you can try to attach a mocked window object to the global scope, e.g. globalThis.window = { getComputedStyle: vi.fn() }

@Pawel1894
Copy link
Contributor Author

Pawel1894 commented Nov 3, 2023

@Pawel1894 for this test you can try to attach a mocked window object to the global scope, e.g. globalThis.window = { getComputedStyle: vi.fn() }

So i tried what you recommended, also this but with mockReturnValue and mockImplementation and I always end up with empty object returned from execute function.

So I end up with this

TypeError: Cannot read properties of undefined (reading 'padding-top')
  eval __mocks__/got.ts:206:24
    204|     case `/session/${sessionId}/execute`:
    205|     case `/session/${sessionId}/execute/sync`: {
    206|         const script = Function(params.json.script)

vi.spyOn(browser, 'execute').mockReturnValue('12px') <- this worked for me but it won't allow me to write test for shorthand property, so I'm not happy with this solution either.

@christian-bromann
Copy link
Member

also this but with mockReturnValue and mockImplementation

I am not sure if you need these. Instead make sure the mock (./__mocks__/got.ts) returns the expected value for your needs. Also feel free to stub the browser objects completely if you don't want to use the mock.

@Pawel1894
Copy link
Contributor Author

Pawel1894 commented Nov 6, 2023

also this but with mockReturnValue and mockImplementation

I am not sure if you need these. Instead make sure the mock (./__mocks__/got.ts) returns the expected value for your needs. Also feel free to stub the browser objects completely if you don't want to use the mock.

@christian-bromann Tbh it have really hard time trying to understand how code in mocks got.ts works. Do you mean by feel free to stub the browser objects to mock the getPseudoElementCSSValue?

@christian-bromann christian-bromann added the PR: New Feature 🚀 PRs that contain new features label Nov 8, 2023
Copy link
Member

@christian-bromann christian-bromann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will go ahead and merge this and will add necessary unit tests as an additional PR.

Thanks for the contribution 👍

@christian-bromann christian-bromann merged commit b109450 into webdriverio:main Nov 8, 2023
8 checks passed
@christian-bromann
Copy link
Member

See d68bc2e

@Pawel1894
Copy link
Contributor Author

@christian-bromann Thx 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: New Feature 🚀 PRs that contain new features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants