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

Click on download and print buttons using element.click function in scripting integration tests #12819

Closed
wants to merge 1 commit into from

Conversation

calixteman
Copy link
Contributor

@calixteman calixteman commented Jan 6, 2021

If for any reason the save/print buttons aren't on screen (no height), the page.click is failing.
So replace page.click by document.querySelector(...).click().

@calixteman
Copy link
Contributor Author

/botio integrationtest

@pdfjsbot
Copy link

pdfjsbot commented Jan 6, 2021

From: Bot.io (Linux m4)


Received

Command cmd_integrationtest from @calixteman received. Current queue size: 0

Live output at: http://54.67.70.0:8877/427409369e3c7ab/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Jan 6, 2021

From: Bot.io (Windows)


Received

Command cmd_integrationtest from @calixteman received. Current queue size: 0

Live output at: http://3.101.106.178:8877/bb64d23f46cacf1/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Jan 6, 2021

From: Bot.io (Linux m4)


Success

Full output at http://54.67.70.0:8877/427409369e3c7ab/output.txt

Total script time: 2.88 mins

  • Integration Tests: Passed

@pdfjsbot
Copy link

pdfjsbot commented Jan 6, 2021

From: Bot.io (Windows)


Failed

Full output at http://3.101.106.178:8877/bb64d23f46cacf1/output.txt

Total script time: 3.64 mins

  • Integration Tests: FAILED

@calixteman
Copy link
Contributor Author

/botio integrationtest

@pdfjsbot
Copy link

pdfjsbot commented Jan 6, 2021

From: Bot.io (Windows)


Received

Command cmd_integrationtest from @calixteman received. Current queue size: 0

Live output at: http://3.101.106.178:8877/4082b831b8f46be/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Jan 6, 2021

From: Bot.io (Linux m4)


Received

Command cmd_integrationtest from @calixteman received. Current queue size: 0

Live output at: http://54.67.70.0:8877/33ab3ffb7d9171e/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Jan 6, 2021

From: Bot.io (Linux m4)


Success

Full output at http://54.67.70.0:8877/33ab3ffb7d9171e/output.txt

Total script time: 2.78 mins

  • Integration Tests: Passed

@pdfjsbot
Copy link

pdfjsbot commented Jan 6, 2021

From: Bot.io (Windows)


Failed

Full output at http://3.101.106.178:8877/4082b831b8f46be/output.txt

Total script time: 4.23 mins

  • Integration Tests: FAILED

@calixteman
Copy link
Contributor Author

/botio integrationtest

@pdfjsbot
Copy link

pdfjsbot commented Jan 6, 2021

From: Bot.io (Linux m4)


Received

Command cmd_integrationtest from @calixteman received. Current queue size: 0

Live output at: http://54.67.70.0:8877/13e3b31da185bff/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Jan 6, 2021

From: Bot.io (Windows)


Received

Command cmd_integrationtest from @calixteman received. Current queue size: 0

Live output at: http://3.101.106.178:8877/6f4f7c2738da5c9/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Jan 6, 2021

From: Bot.io (Linux m4)


Success

Full output at http://54.67.70.0:8877/13e3b31da185bff/output.txt

Total script time: 2.82 mins

  • Integration Tests: Passed

@pdfjsbot
Copy link

pdfjsbot commented Jan 6, 2021

From: Bot.io (Windows)


Success

Full output at http://3.101.106.178:8877/6f4f7c2738da5c9/output.txt

Total script time: 3.69 mins

  • Integration Tests: Passed

@calixteman calixteman changed the title Be sure to have download and print buttons visible in scripting integration tests Click on download and print buttons using element.click function in scripting integration tests Jan 6, 2021
@calixteman calixteman added the test label Jan 6, 2021
@calixteman
Copy link
Contributor Author

/botio integrationtest

@pdfjsbot
Copy link

pdfjsbot commented Jan 8, 2021

From: Bot.io (Windows)


Received

Command cmd_integrationtest from @calixteman received. Current queue size: 0

Live output at: http://3.101.106.178:8877/5467857c4cac5c1/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Jan 8, 2021

From: Bot.io (Linux m4)


Received

Command cmd_integrationtest from @calixteman received. Current queue size: 0

Live output at: http://54.67.70.0:8877/f058f1f3ae6a2a9/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Jan 8, 2021

From: Bot.io (Linux m4)


Success

Full output at http://54.67.70.0:8877/f058f1f3ae6a2a9/output.txt

Total script time: 2.85 mins

  • Integration Tests: Passed

@pdfjsbot
Copy link

pdfjsbot commented Jan 8, 2021

From: Bot.io (Windows)


Success

Full output at http://3.101.106.178:8877/5467857c4cac5c1/output.txt

Total script time: 3.84 mins

  • Integration Tests: Passed

@timvandermeij
Copy link
Contributor

I'm a bit confused since the integration test that apparently broke in the other PR seems to just pass (as expected) now when I look at #12394 (comment). Is this patch really still necessary given this?

@calixteman
Copy link
Contributor Author

When the test was failing, I had an open connection through rdp to the windows bot and it's probably the root cause.
I guess that because of a too small window, the download button had was not rendered.
So I think that this patch is not so bad because if for any reason in the future the layout change and the button is no more visible we'll handle the case.
If you think that's useless to have it, close it, I'm fine with that too.

@timvandermeij
Copy link
Contributor

timvandermeij commented Jan 8, 2021

The Puppeteer API documentation at https://pptr.dev/#?product=Puppeteer&version=v5.5.0&show=api-pageclickselector-options states: "This method fetches an element with selector, scrolls it into view if needed, and then uses page.mouse to click in the center of the element. If there's no element matching selector, the method throws an error."

I'm therefore not really seeing how this is related to the buttons not being visible since Puppeteer will scroll them into view anyway, so therefore I'm not really convinced if a) there is a bug here and b) if this is the right fix. If it can be reproduced, it sounds more like a bug report for Puppeteer (but note also https://stackoverflow.com/questions/51857070/puppeteer-in-nodejs-reports-error-node-is-either-not-visible-or-not-an-htmlele that states that it might also simply be a problem in the test itself). Let's close this for now and if this returns we can reconsider, but given the Puppeteer API information page.click should work as expected.

@calixteman
Copy link
Contributor Author

I tried to wait on button.height !== 0 and the test failed with timeout (30s): it's the reason why I think the button isn't visible.
When the window is too small then the buttons go into the >> menu and so you scroll or do whatever you want you cannot "physically" click on the button it's why using button.click() is working.

@timvandermeij
Copy link
Contributor

timvandermeij commented Jan 8, 2021

In that case, wouldn't setting the viewport as stated on https://stackoverflow.com/questions/51857070/puppeteer-in-nodejs-reports-error-node-is-either-not-visible-or-not-an-htmlele (using page.setViewport) be a better fix here (and in general for all tests, since you want the window to have a reasonable size) or maybe even simply maximizing the entire browser during browser setup (by passing defaultViewport: null according to https://stackoverflow.com/questions/56543232/how-to-maximize-browser-window-using-puppeteer)?

It seems very much like an edge case since the browser are normally not manually touched when the tests run, so if we want to address this I think that's a much simpler fix.

@calixteman
Copy link
Contributor Author

Yeah, I thought about that too but I almost don't know how the bot works (I'm not a windows expert) and what are the available screen size... and I don't really understand why opening the bot through rdp and remmina can change the window size...
So I just had a better feeling in clicking rather than fixing the window size (and for these tests (print and save) I don't care so much to make a "real" click on download/print buttons).
Anyway, I'll do a patch to maximize the browser at startup.

@calixteman
Copy link
Contributor Author

@timvandermeij unfortunately we're starting the browser using defaultViewport: null:

defaultViewport: null,
.

@timvandermeij
Copy link
Contributor

Ugh, it seems like this re-appeared in #12875 and other PRs, so we'll need some solution here. Perhaps we can do this for now, with a clear comment added as to why we do this instead of page.click(), but it doesn't feel like the proper solution. If we do this for now, we should also try to file an upstream issue for Puppeteer since page.click() should work according to the documentation.

@timvandermeij timvandermeij reopened this Jan 20, 2021
@calixteman
Copy link
Contributor Author

I'm pretty sure that's because the window is too small and then the button to save goes to the >> menu and so it's impossible for puppeteer to make the button visible in scrolling so it isn't a problem puppeteer for me but maybe with remmina: I used it friday to restart the bot and problems with integration test came after.

@timvandermeij
Copy link
Contributor

Yes, if that's the case then your explanation makes sense. If the window is too small, the save button is indeed pushed into the secondary toolbar. Since we didn't really have this before, let's indeed keep using page.click() as intended by Puppeteer, but we just need to be careful with bot restarts then. If this really becomes annoying, we can always reconsider something like this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants