Added support for responsive snapshot capture #127
Conversation
chinmay-browserstack
requested review from
ninadbstack and
shantanuk-browserstack
percy/snapshot.py
Outdated
| try: | ||
| # Inject the DOM serialization script | ||
| driver.execute_script(fetch_percy_dom()) | ||
| cookies = driver.get_cookies() | ||
| user_agent = driver.execute_script("return navigator.userAgent;") |
There was a problem hiding this comment.
better to make this change as part of dom package so that we dont have to do it in all sdks + wont need another selenium call
percy/snapshot.py
Outdated
| @@ -18,6 +19,14 @@ | |||
|
|
|||
| # for logging | |||
| LABEL = '[\u001b[35m' + ('percy:python' if PERCY_DEBUG else 'percy') + '\u001b[39m]' | |||
| CDP_SUPPORT_SELENIUM = (str(SELENIUM_VERSION)[0].isdigit() and int( | |||
| str(SELENIUM_VERSION)[0]) >= 4) if SELENIUM_VERSION else False | |||
| fetched_widths = {} | |||
There was a problem hiding this comment.
| fetched_widths = {} | |
| eligible_widths = {} |
percy/snapshot.py
Outdated
| widths = kwargs.get('widths') or [] | ||
| if 'width' in kwargs: | ||
| widths = [kwargs.get('width')] | ||
|
|
||
| # cache doesn't work when parameter is a list so passing tuple | ||
| widths = get_widths_for_multi_dom(tuple(widths)) |
There was a problem hiding this comment.
refactor this handle widths completely. Idea is to have all the variables declared in single line at the start of functions.
percy/snapshot.py
Outdated
|
|
||
| for width in widths: | ||
| change_window_dimension(driver, width, current_height) | ||
| sleep(1) |
There was a problem hiding this comment.
eh doesn't it mean we are slowing it by 1 sec. Use wait of selenium to ensure that command is over. Also command is sync in nature by default.
There was a problem hiding this comment.
page is already loaded so can't rely on page load / element visible event. i am adding wait so any change in DOM due to JS with take place. are you talking about selenium implicit wait?
percy/snapshot.py
Outdated
|
|
||
| # Post the DOM to the snapshot endpoint with snapshot options and other info | ||
| response = requests.post(f'{PERCY_CLI_API}/percy/snapshot', json={**kwargs, **{ | ||
| 'client_info': CLIENT_INFO, | ||
| 'environment_info': ENV_INFO, | ||
| 'environment_info': ENV_INFO + [user_agent], |
There was a problem hiding this comment.
This will change FE info as well.
There was a problem hiding this comment.
removed and added log line in cli instead
There was a problem hiding this comment.
We dont need this for log line - we need to add implicit requestHeader using this in long term
There was a problem hiding this comment.
so should we send this as separate field in API? this will need api change as well.
| try: | ||
| # Inject the DOM serialization script | ||
| driver.execute_script(fetch_percy_dom()) | ||
| cookies = driver.get_cookies() |
There was a problem hiding this comment.
This will be a list of dictionaries and browser expects it in different format we need to add them in proper format. We would need to handle it in CLI otherwise.
There was a problem hiding this comment.
This is also correct format here it return { 'name': '', 'value': ''} in this format and our cli works well with it.
There was a problem hiding this comment.
Thats not the current format of the cookies in CLI though. once that are coming from document.cookies
There was a problem hiding this comment.
Since this check will not become true so cookies are in correct format only as we need to pass in cdp
chinmay-browserstack
deleted the
responsive_snapshot_capture_support
branch
responsiveSnapshotCapture. when this option is passed as true, we will capture dom in all widths returned by CLI.set_window_sizefor selenium 3 or non chrome browsers else using cdp to resize window. reason: in chrome we can only resize window upto 500px so to bypass this we are using cdp but running cdp command in selenium 3 is not supported./percy/logendpoint