Fix gnome_randr_cycle not working under Wayland (LP:#2036172)(BugFix)

[hugh712]

Description

• Change the position of current_modes to earlier.
• Compare rate and existing_rate with float instead of string.

Resolved issues

Currently runs 'gnome_randr_cycle.py' it will just directly finish without toggling any resolution, because some code glitch, after my change, it will work as expected.

Tests

I run code as below on my 22.04 laptop :

$ python3 /usr/lib/checkbox-provider-base/bin/gnome_randr_cycle.py --screenshot-dir=~/tmp
Set mode 1368x768@119.83 for output eDP-1
Set mode 1600x1200@89.91 for output eDP-1
Set mode 1920x1080@89.93 for output eDP-1
Set mode 1920x1200@120.00 for output eDP-1
Set mode 1920x1200@60.00 for output eDP-1

It will

• Pick up a highest refresh rate in each resolution
• Find the top resolution per aspect
• Change resolutions and get screenshots,
• Finally change back to original resolution, everything works as expected.

[hugh712]

@pieqq

Would you please help to review my fix? thank you :)

[pieqq]

@hugh712 thanks for this patch!

It seems your modifications are not passing the flake8 tests. From the tox run:

FAIL: test_flake8_/home/runner/work/checkbox/checkbox/providers/base/bin/gnome_randr_cycle.py (plainbox.provider_manager.Flake8Tests)

I added a few comments inline.

Also, did you check for regression? Have you tested your fix on X11? On 20.04?

[hugh712]

@pieqq

Thanks for the review :)

For the question

Also, did you check for regression? Have you tested your fix on X11?

As I know, this gnome_randr_cycle was designed for wayland, and xrandr_cycle.py for X11,
So I am not sure to verify this gnome_randr_cycle under X11 is a valid test case?

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 9 lines in your changes missing coverage. Please review.

Project coverage is 35.69%. Comparing base (4319b97) to head (e6ab7b4).
Report is 576 commits behind head on main.

Files with missing lines	Patch %	Lines
providers/base/bin/gnome_randr_cycle.py	0.00%	9 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #855      +/-   ##
==========================================
- Coverage   35.69%   35.69%   -0.01%     
==========================================
  Files         303      303              
  Lines       34250    34251       +1     
  Branches     5917     5917              
==========================================
  Hits        12226    12226              
- Misses      21459    21460       +1     
  Partials      565      565              

Flag	Coverage Δ
provider-base	5.29% <0.00%> (-0.01%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[pieqq]

Thanks for the changes!

gnome-randr is compatible with both X11 and Wayland in theory, that's why I asked if you had done some regression testing, but after checking in our providers, we indeed only run this script when using Wayland, so no need to do this.

However, currently gnome_randr_cycle.py is not covered by any unit test. This is now a requirement for new PRs, as explained in the Testing section of the Contributing Guide.

Given that this script is a bit long and written in a very "imperative programming" style (I mean no functions, no classes, just running it line by line), it's gonna be difficult to test. I recommend you split the different parts into functions that should be easier to write unit tests for.

Reach out if you need help!

[pieqq]

@hugh712 Hey! Did you have time to look into this to add some unit tests for your script? There are a few sections in the contrib guide that may help you:

• https://github.com/canonical/checkbox/blob/main/CONTRIBUTING.md#writing-and-running-unit-tests-for-providers
• https://github.com/canonical/checkbox/blob/main/CONTRIBUTING.md#coverage

[hugh712]

@hugh712 Hey! Did you have time to look into this to add some unit tests for your script? There are a few sections in the contrib guide that may help you:

• https://github.com/canonical/checkbox/blob/main/CONTRIBUTING.md#writing-and-running-unit-tests-for-providers
• https://github.com/canonical/checkbox/blob/main/CONTRIBUTING.md#coverage

@pieqq

Hi Pierre,
This is still in my pocket, but unfortunately still can not find time to do that,
will let you know once I have progress, thank you :)

[kissiel]

This PR awaits the author to have some time to work on it, and there's still some missing bits, so I'm switching the state to "Draft"

[Hook25]

Closing this as we completely changed how we set the resolution, fetch resolution but most importantly, it is basically inactive. Feel free to re-open it