Skip to content

Set the auto scale method explicitly in ImagesWin32Tests #1807

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fedejeanne
Copy link
Contributor

@fedejeanne fedejeanne commented Feb 5, 2025

Rename the test class and let it test the "smooth" scale method only, which was the root cause of the problem all along.

Contributes to #1790

Copy link
Contributor

github-actions bot commented Feb 5, 2025

Test Results

   502 files  ±0     502 suites  ±0   10m 38s ⏱️ + 1m 26s
 4 334 tests ±0   4 320 ✅ ±0   14 💤 ±0  0 ❌ ±0 
16 575 runs  ±0  16 466 ✅ ±0  109 💤 ±0  0 ❌ ±0 

Results for commit 7250aa9. ± Comparison against base commit 920fe85.

This pull request removes 1 and adds 1 tests. Note that renamed tests count towards both.
org.eclipse.swt.graphics.ImagesWin32Tests ‑ testImageIconTypeShouldNotChangeAfterCallingGetHandleForDifferentZoom
org.eclipse.swt.graphics.ImageSmoothScalingWin32Tests ‑ testImageIconTypeShouldNotChangeAfterCallingGetHandleForDifferentZoom

♻️ This comment has been updated with latest results.

Copy link
Contributor

@HeikoKlare HeikoKlare left a comment

Choose a reason for hiding this comment

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

Some thoughts on this:

  • The test is somehow specific to "smooth" scaling (at least the test was introduced as a regression test related to that scaling mode).
  • The test class is annotated with the WithMonitorSpecificScalingExtension to enable monitor-specific scaling, which, in turn, enables "smooth" scaling.

So given that, I would expect one of the following solution:

  • Make the test specific to "smooth" scaling, which would include renaming the test and removing the extension
  • Make monitor-specific scaling properly enable smooth mode (for which we have a dedicated issue), so that the additional setup logic for "smooth" scaling is not required.

@fedejeanne fedejeanne force-pushed the set_autoscale_method_in_test branch from ff976a3 to 50b9b0a Compare February 5, 2025 15:37
- Rename ImagesWin32Tests to ImageSmoothScalingWin32Tests
- Set system property "swt.autoScale.method=smooth" so the test works as
a proper regression test.

Contributes to
eclipse-platform#1790
@fedejeanne fedejeanne force-pushed the set_autoscale_method_in_test branch from 50b9b0a to 7250aa9 Compare February 5, 2025 15:38
@fedejeanne
Copy link
Contributor Author

I went for option nr. 1:

* Make the test specific to "smooth" scaling, which would include renaming the test and removing the extension

Why?
Removing @ExtendWith(WithMonitorSpecificScalingExtension.class) makes it work a proper regression test, which means that we were testing the smooth scaling behavior all along without knowing it. I double checked this assumption by switching the VM parameter -Dswt.autoScale.method between nearest and smooth and again: nearest lets the test pass always and smooth doesn't (i.e., it behaves like a regression test).

@fedejeanne fedejeanne marked this pull request as ready for review February 5, 2025 15:41
Copy link
Contributor

@HeikoKlare HeikoKlare left a comment

Choose a reason for hiding this comment

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

Changing the test to focus on smooth scaling is fine for me, but the current solution will not work. SWT_AUTOSCALE_METHOD is read only once in the static initialized of DPIUtil. There is no guarantee that static intializer is executed after the setup method of the test class. And even if you were able to ensure that, the current restoration logic will not apply as the system property that is reset will not be re-evaluated afterwards.

@fedejeanne
Copy link
Contributor Author

Hm, you're right: as it is, this test passes only by chance and it doesn't restore the previous auto scale method. I guess we will need to wait until the auto scale method can be properly set in runtime (if that's what we want) or we can force the auto scale method somehow, probably with a new method in DPIUtil.

I'll postpone this discussion (and this PR) until we discuss how to proceed with vi-eclipse/Eclipse-Platform#227, which is related.

@fedejeanne fedejeanne marked this pull request as draft February 5, 2025 15:58
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