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

Run fluxbox as window manager for jobs using Xvfb #11025

Merged
merged 1 commit into from Sep 25, 2022

Conversation

whimboo
Copy link
Contributor

@whimboo whimboo commented Sep 13, 2022

PR for regression testing for mozilla/geckodriver#2042.

@codecov-commenter
Copy link

codecov-commenter commented Sep 13, 2022

Codecov Report

Base: 53.18% // Head: 53.18% // No change to project coverage 👍

Coverage data is based on head (8d115d9) compared to base (b2598e7).
Patch has no changes to coverable lines.

Additional details and impacted files
@@           Coverage Diff           @@
##            trunk   #11025   +/-   ##
=======================================
  Coverage   53.18%   53.18%           
=======================================
  Files          84       84           
  Lines        5488     5488           
  Branches      272      272           
=======================================
  Hits         2919     2919           
  Misses       2297     2297           
  Partials      272      272           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@titusfortner
Copy link
Member

sounds like we found the culprit!

@whimboo
Copy link
Contributor Author

whimboo commented Sep 14, 2022

With the latest push the failure seem to be fixed. We are in process to decide which patches exactly need to be backed out. So once done and a Nightly build shipped I would like to push one more time to this PR for verification.

It's really interesting that this issue can only be seen in GitHub Actions.

@whimboo whimboo force-pushed the firefox_window_position branch 9 times, most recently from 8ab647c to 56e0647 Compare September 15, 2022 20:13
@whimboo whimboo changed the title Testing failure in setting window position with firefox Run fluxbox as window manager for jobs using Xvfb Sep 15, 2022
@whimboo whimboo force-pushed the firefox_window_position branch 6 times, most recently from 7625700 to 131912e Compare September 19, 2022 18:44
@whimboo whimboo force-pushed the firefox_window_position branch 2 times, most recently from 972ca42 to 480c153 Compare September 19, 2022 19:50
@whimboo
Copy link
Contributor Author

whimboo commented Sep 20, 2022

@titusfortner I could actually need your help here for the failure in the Ruby bindings which is related to the assumption that a window manager would set the DESKTOP_SESSION. Like the following tests work now with a window manager running:

  1) Selenium::WebDriver::Window can maximize the current window FIXED
     Expected pending 'Test guarded; no reason given' to fail. No error was raised.
     # ./rb/spec/integration/selenium/webdriver/window_spec.rb:108

  2) Selenium::WebDriver::Window can make window full screen FIXED
     Expected pending 'Test guarded; no reason given' to fail. No error was raised.
     # ./rb/spec/integration/selenium/webdriver/window_spec.rb:119

  3) Selenium::WebDriver::Window can minimize the window FIXED
     Expected pending 'Test guarded; no reason given' to fail. No error was raised.
     # ./rb/spec/integration/selenium/webdriver/window_spec.rb:130

I wonder if we should simply set the DESKTOP_SESSION=fluxbox environment variable manually given that Fluxbox isn't doing that, or removing this particular behavior completely and always enforce that a window manager is needed to run the browser tests. Without one all the window manipulation tests aren't guaranteed to work.

Regarding the Java failures I would suggest we defer until Ruby is done. Thanks

@whimboo
Copy link
Contributor Author

whimboo commented Sep 20, 2022

I wonder if we should simply set the DESKTOP_SESSION=fluxbox environment variable manually given that Fluxbox isn't doing that, or removing this particular behavior completely and always enforce that a window manager is needed to run the browser tests. Without one all the window manipulation tests aren't guaranteed to work.

If we need a way to detect a window manager a better way should be the following:

xprop -root -display :99

If it's not failing to connect to the display and no window manager is installed we should expect a couple of _NET_* lines to be returned.

@titusfortner
Copy link
Member

No, this is great. We only guarded them that way because I want to be able to run locally on linux and remotely on travis/github and have everything "pass" for the right reasons. If everything guarded for window_manager now works, we can completely remove that guard.

@whimboo
Copy link
Contributor Author

whimboo commented Sep 20, 2022

There is one Java test that now works with Firefox 105. I've created a separate PR: #11046.

@whimboo
Copy link
Contributor Author

whimboo commented Sep 21, 2022

There are quite a few failures for Java on trunk as well. Those started between September 13th and 15th:
https://github.com/SeleniumHQ/selenium/actions/runs/3092869938/jobs/5004771191

The failures look similar to me so I don't think that this PR should be blocked on them. @titusfortner if you could have a look and review this PR I would appreciate.

@sonarcloud
Copy link

sonarcloud bot commented Sep 25, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@titusfortner titusfortner merged commit 8aeb1e1 into SeleniumHQ:trunk Sep 25, 2022
@whimboo whimboo deleted the firefox_window_position branch September 26, 2022 06:00
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.

None yet

3 participants