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

Added minimize keyword #1741

Merged
merged 3 commits into from
Nov 25, 2023
Merged

Conversation

lmartorella
Copy link
Contributor

Added the access to minimize method, symmetrical to the maximize one.

@aaltat
Copy link
Contributor

aaltat commented Aug 30, 2021

Please write test, acceptance or unit, for the new keyword.

@lmartorella
Copy link
Contributor Author

Hello, I would like to edit/copy the tests for the existing maximize_browser_window, the sister keyword. Could you please point me to the acceptance tests for that one?
Thx, L

@aaltat
Copy link
Contributor

aaltat commented Aug 30, 2021

It seems that we do not have one. But easy way would be to use https://robotframework.org/SeleniumLibrary/SeleniumLibrary.html#Set%20Window%20Size and set window size something small Then use new keyword and verify that size is bigger.

@lmartorella
Copy link
Contributor Author

Added an acceptance test, using the underlying selenium tests as inspiration. However it seems that even in the selenium library such tests are disabled, perhaps due to instability or unable to access to the browser's maximized/minimized state on all the platforms and OSes.

The minimize test is only checking for windows.hidden. It seems that accessing to window position and size doesn't provide correct information (position unchanged, horizontal size changes to a random value but height is not).
The maximize test is more robust and size/pos can be used as reference.

@aaltat
Copy link
Contributor

aaltat commented Sep 1, 2021

Looks good for me. @emanlove what you want to do with the PR?

@lmartorella
Copy link
Contributor Author

Hi, I've rebased the branch, but I don't fully understand how to make the checks to pass.

@emanlove
Copy link
Member

emanlove commented Oct 9, 2022

Checking against latest version of unit and acceptance tests but failing tests. Investigating at the moment and wondering if we are running in headless mode if this affects the Page Visibility correctness in reporting .. Did note that the Page Visibility API is supported by all major browsers so concern about browser support shouldn't be an issue.

@lmartorella
Copy link
Contributor Author

Mmm yes, it seems that the Windows suite runs in headless mode (browser created only once in the Suite Setup), so it will be quite hard to test the Minimize behavior there. There are no explicit documentation about Visibility API and headless mode interaction.
Probably the best approach for such specific test is to run without the --headless flag, but I'm not sure if this violates some policy of the test suite.

@emanlove
Copy link
Member

Was relooking at this and thought to see/ask what the Selenium project is doing to test this. Titus Fortner noted that one of their contributors added fluxbox to their GitHub actions to get minimize working. I am going to check out their GitHub Actions CI configuration and try out Titus's suggestion.

@lmartorella
Copy link
Contributor Author

Hi,
Is there any chance to have this reviewed?
The proposed code is barely a plain forwarded call to the lib, with no arguments, no complexity, complementary to the already existing "Maximize" (so quite expected to be there).

I expect that the base Selenium library is already offering the required code quality, since offered to a broader set of libraries.
I understand for sure the need of a E2E validation, but having such a basic PR blocked for two years is quite sad.

@emanlove
Copy link
Member

@lmartorella Sorry, this slipped between the cracks and I lost track of this. Putting this on the next release's milestone.

@emanlove emanlove added this to the V6.2.0 milestone Sep 13, 2023
@emanlove emanlove modified the milestones: v6.2.0, v6.3.0 Nov 19, 2023
@emanlove emanlove merged commit 82fbe22 into robotframework:master Nov 25, 2023
0 of 9 checks passed
@emanlove emanlove added enhancement priority: high acknowledge To be acknowledged in release notes and removed in progress labels Nov 25, 2023
@emanlove emanlove added the rc 1 label Mar 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
acknowledge To be acknowledged in release notes enhancement priority: high rc 1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants