Skip to content

Conversation

Tieqiong
Copy link
Contributor

@Tieqiong Tieqiong commented Jan 16, 2025

@sbillinge please check, thanks

This is not a fix on the actual code. This is for fixing a buggy test code (bug on test code not on the actual code) that fails all the windows matrix test.

The problem is on test_LogoClicks in test_aboutdialog.py. This test is to simulate user interactions with the About tag in GUI and verify clicking on various logo buttons leads to the expected url. The modified context manager overridewebbrowser is in testutils but used in test_LogoClicks. It try to temporarily replaces the normal behavior of opening a web browser by calling set_url which captures the url that pdfgui tries to open without actually opening the browser.

The reason why the test was not passing only on Windows is because webbrowser.open work differently on windows, which returns different instance for subsequent calls. Thus the override in the old test code doesn't stick to the right instance as the code that actually calls webbrowser.open ends up using a different unchanged controller.

Copy link

codecov bot commented Jan 16, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.10%. Comparing base (1cb1890) to head (f23184b).
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #255      +/-   ##
==========================================
- Coverage   95.11%   95.10%   -0.02%     
==========================================
  Files          23       23              
  Lines        1126     1123       -3     
==========================================
- Hits         1071     1068       -3     
  Misses         55       55              
Files with missing lines Coverage Δ
tests/testutils.py 98.18% <100.00%> (-0.05%) ⬇️

@sbillinge
Copy link
Contributor

@sbillinge please check, thanks

This is not a fix on the actual code. This is for fixing a buggy test code (bug on test code not on the actual code) that fails all the windows matrix test.

The problem is on test_LogoClicks in test_aboutdialog.py. This test is to simulate user interactions with the About tag in GUI and verify clicking on various logo buttons leads to the expected url. The modified context manager overridewebbrowser is in testutils but used in test_LogoClicks. It try to temporarily replaces the normal behavior of opening a web browser by calling set_url which captures the url that pdfgui tries to open without actually opening the browser.

The reason why the test was not passing only on Windows is because webbrowser.open work differently on windows, which returns different instance for subsequent calls. Thus the override in the old test code doesn't stick to the right instance as the code that actually calls webbrowser.open ends up using a different unchanged controller.

again, thanks @Tieqiong for figuring this out (this bug was really "bugging" me for a long time!) but also for the detailed explanation!

@sbillinge sbillinge merged commit bf4873c into diffpy:main Jan 16, 2025
5 checks passed
@Tieqiong Tieqiong deleted the fixwin branch January 17, 2025 03:34
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.

2 participants