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

Default to a 4K x 4K browser window #110

Merged
merged 3 commits into from
May 14, 2019
Merged

Conversation

Rotonen
Copy link
Contributor

@Rotonen Rotonen commented May 11, 2019

The largest categories of Robot test flakiness we have are:

  1. Things being unclickable by being outside the browser viewport
  2. Interactive elements, like form buttons, collapsing into themselves in a way where they overlap in a mobile view

Both are caused by the default window size being a rather arcane 800 x 600. (Or in the case of Xvfb, also limited by the default desktop size and color depth of 640x480x8). In some cases, with Xvfb, the browser defaults, for unknown reasons, to a window size of 1024x768. None of these represent contemporary actual use very well.

The usability issues, especially the mobile ones, can be real usability issues and should thus be eventually tackled. I deem stable tests more valuable at this point in time. Currently these issues manifest mostly for people whom are not tackling them and they simply demotivate contributors.

Tackling those has also been a very annoying heisenbug for developers, as the browser window sizes on their local runs are different than on the CI as on desktop environments the default browser size is larger than in a naked X11 session. And someone with a smaller screen, for example on a smaller laptop, cannot even set their browser window size very large and is currently stuck with flakier Robot tests.

Thus we also need to use a headless browser without Xvfb for both the CI runs and local development. The support for this is already present in geckodriver, chromedriver, the version of Selenium we use, the version of Robot framework we use and thus also in our testing stack. Using a headless browser for local development is also a more pleasant experience as it does not spawn windows (some of which pop onto the foreground and gain focus). This allows the developer to do something else while waiting for the tests to run.

If there is just a single place from where the viewport size can be controlled, anyone interested in tackling these issues can do a local checkout of plone.app.robotframework and set the viewport size to whatever they're interested in working on.

I've figured out the path to get to that point and it starts here.

Attending to this one will happen in multiple parts:

  1. Set the window size in plone.app.robotframework to a suitably unrealistic 4K x 4K
  2. Remove window size setting from all Robot test layers and tests
  3. Use the same shared test setup in all Robot tests
  4. Use headless browsers on the CI instead of xvfb-run wrapped windowed browsers
  5. Default to headless browsers for all Robot testing in Plone

For anyone interested in giving the headless browsers a spin locally:
ROBOT_BROWSER=headlesschrome
ROBOT_BROWSER=headlessfirefox

@mister-roboto

This comment has been minimized.

@Rotonen Rotonen force-pushed the roto-window-resize-huge branch from df64c4c to 736e29e Compare May 11, 2019 10:35
@Rotonen
Copy link
Contributor Author

Rotonen commented May 11, 2019

@jenkins-plone-org please run jobs

@Rotonen Rotonen force-pushed the roto-window-resize-huge branch from 736e29e to f65a340 Compare May 11, 2019 11:23
@Rotonen
Copy link
Contributor Author

Rotonen commented May 11, 2019

@jenkins-plone-org please run jobs

@Rotonen
Copy link
Contributor Author

Rotonen commented May 11, 2019

Seems I'll also need to convert the tests and documentation of this package itself. Apparently Products.CMFPlone is just about the only package which uses the shared setups from here.

@Rotonen Rotonen force-pushed the roto-window-resize-huge branch from 60f25fe to ccdbc1d Compare May 11, 2019 12:40
@Rotonen Rotonen force-pushed the roto-window-resize-huge branch from ccdbc1d to 090d6fe Compare May 11, 2019 13:07
@Rotonen
Copy link
Contributor Author

Rotonen commented May 11, 2019

I'm deliberately not yet running the tests on the Plone CI now as I'm hunting for what broke in plone/plone.dexterity#102 for Python 2. After the 5.2 branch of buildout.coredev is green again, I'll resume hunting for all things Robot.

@Rotonen
Copy link
Contributor Author

Rotonen commented May 13, 2019

@jenkins-plone-org please run jobs

Copy link
Member

@jensens jensens left a comment

Choose a reason for hiding this comment

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

LGTM, merge yourself if you think its ready!

@Rotonen
Copy link
Contributor Author

Rotonen commented May 13, 2019

I'll wait for the Zope 4 dust to settle first at least. @pbauer is currently on that one.

I want to be increasing the certainty and reliability of the test runs.

I'm not in a particular hurry to merge, this will take many pull requests to get everything done and as our CI infra elegantly allows the testing of combinations of PRs, I can work towards the end goal with or without some parts of the puzzle having been merged.

I'll merge this when we have a green 5.2 again and I have a few subsequent PRs tested against this one.

@Rotonen
Copy link
Contributor Author

Rotonen commented May 14, 2019

As the layer PRs are now in, I'll merge this and start requesting reviews for the ones which are only tested against 5.2 on the master branch.

I'll later need to port this to at least 5.1 as well.

@Rotonen Rotonen merged commit 5d894c7 into master May 14, 2019
@Rotonen Rotonen deleted the roto-window-resize-huge branch May 14, 2019 10:54
@mauritsvanrees
Copy link
Member

I have released plone.app.robotframework 1.5.2 with this change.

Tiny note: please do not add dashes in front of the news snippet files. They get included literally when towncrier runs, and towncrier adds its own dashes so you end up with double dashes.
And please only use one snippet per file. If you want more items in the resulting changelog, you can put them in separate files, like I did here before the release.

@Rotonen
Copy link
Contributor Author

Rotonen commented May 21, 2019

@mauritsvanrees thank you. First time driving towncrier. I'll amend all the other PRs.

@thet
Copy link
Member

thet commented Jun 30, 2019

At the Buschenschanksprint we encountered high performance requirements for running the Jenkins Robot tests on a node we own.
The 4k change might make the browsers require up more CPU/RAM resources than before.

Just as a side note.

@Rotonen
Copy link
Contributor Author

Rotonen commented Jul 2, 2019

@thet using a headless browser without Xvfb has negated both extra CPU and GPU use in testing.

See and track also plone/jenkins.plone.org#259

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

Successfully merging this pull request may close these issues.

5 participants