Removing getDeviceZoom from tests #2141
Conversation
arunjose696
force-pushed
the
arunjose696/195
branch
from
d1d296f to
046b829
Compare
arunjose696
force-pushed
the
arunjose696/195
branch
2 times, most recently
from
42a81c2 to
93ec625
Compare
arunjose696
force-pushed
the
arunjose696/195
branch
from
93ec625 to
5eb4ab0
Compare
arunjose696
changed the title
There was a problem hiding this comment.
In general, the changes look sound to me. The removal of the assumptions for deviceZoom being 100 in some of the tests seems not okay to me, as the tests fail on specific zooms, but maybe the assumptions themselves can be improved.
@akoch-yatta would be nice if you could still have another look to ensure that the changes are sound and that we do not degrade the tests
tests/org.eclipse.swt.tests/JUnit Tests/org/eclipse/swt/tests/junit/DPIUtilTests.java
Outdated
Show resolved
Hide resolved
| if (DPIUtil.getDeviceZoom() != 100) { | ||
| //TODO Fix non integer scaling factors. | ||
| if (SwtTestUtil.verbose) { | ||
| System.out.println("Excluded test_copyAreaIIIIII(org.eclipse.swt.tests.junit.Test_org_eclipse_swt_graphics_GC)"); | ||
| } | ||
| return; | ||
| } |
There was a problem hiding this comment.
Are we sure that this can this safely be removed, i.e, will the test run properly on with deviceZoom != 100 on every OS? When I execute this test on Windows with 175% primary monitor zoom, it fails (probably because of fractional scaling rounding errors). So maybe we should rather replace this with an according JUnit assumption but in general keep the check.
There was a problem hiding this comment.
I tested this on Windows 11 with various zoom levels (100%, 125%, 150%, 175%, 200%, and 225%) and observed that the tests passed in all cases, which is why I removed the check.
Could you share the exact error you're encountering at 175%—is it an assertion failure?
Also, are you using any specific arguments in your run configuration?
If we replace the check with a JUnit assumption, don’t we still need to call DPIUtil.getDeviceZoom()? Or is there another way to get the primary monitor’s zoom level that works across platforms?
There was a problem hiding this comment.
I just found that I cannot reproduce the issue with your exact branch, but when rebasing it on top of the lastest master branch state. I guess we made some adaptations since you last rebased this branch on master that affect these tests.
When executing the tests at 175% primary monitor zoom on that state, I get this error:
java.lang.AssertionError: :b: expected:<RGB {0, 0, 255}> but was:<RGB {255, 255, 255}>
at org.junit.Assert.fail(Assert.java:89)
at org.junit.Assert.failNotEquals(Assert.java:835)
at org.junit.Assert.assertEquals(Assert.java:120)
at org.eclipse.swt.tests.junit.Test_org_eclipse_swt_graphics_GC.test_copyAreaIIIIII(Test_org_eclipse_swt_graphics_GC.java:160)
at java.base/java.lang.reflect.Method.invoke(Method.java:568)
If we replace the check with a JUnit assumption, don’t we still need to call DPIUtil.getDeviceZoom()? Or is there another way to get the primary monitor’s zoom level that works across platforms?
Yes, we would then still need to use DPIUtil.getDeviceZoom(). But our primary goal it to reevaluate all existing usages whether they can be replaced or whether they are fine and we may consider it fine to be used here as we are not in the situation that the returned value is useless because of potentially having multiple shells placed at different monitors with different zoom values on Windows (which is actually the use case in which the usage of get[Native]DeviceZoom() fails).
There was a problem hiding this comment.
we may consider it fine to be used here as we are not in the situation that the returned value is useless because of potentially having multiple shells placed at different monitors with different zoom values on Windows (which is actually the use case in which the usage of
get[Native]DeviceZoom()fails).
Yeah makes sense, I will modify this to junit assumptions
There was a problem hiding this comment.
Replaced the checks with junit assumption
arunjose696
force-pushed
the
arunjose696/195
branch
3 times, most recently
from
4df2bb1 to
dc189d5
Compare
|
This pull request changes some projects for the first time in this development cycle. An additional commit containing all the necessary changes was pushed to the top of this PR's branch. To obtain these changes (for example if you want to push more changes) either fetch from your fork or apply the git patch. Git patchFurther information are available in Common Build Issues - Missing version increments. |
arunjose696
force-pushed
the
arunjose696/195
branch
from
dc189d5 to
4d57f3d
Compare
HeikoKlare
force-pushed
the
arunjose696/195
branch
from
4d57f3d to
2f5127d
Compare
…nd tests The DPIUtil.get[Native]DeviceZoom() has no proper meaning when using monitor-specific scaling.
HeikoKlare
force-pushed
the
arunjose696/195
branch
from
2f5127d to
05322ef
Compare
DPIUtil.get[Native]DeviceZoom() is no longer meaningful when monitor-specific scaling is used, so its usage has been removed from most tests. In some cases , these have been left unchanged for now.
Among the occurrences mentioned in Eclipse Platform Issue #195, most have been addressed. Still the following four cases remain unmodified:
In these three tests,
getDeviceZoom()is used to capture the zoom level before execution and restore it afterward. No suitable replacement has been identified yet.The snippet description says it is used to to display/refresh Monitor DPI dynamically. However, it uses incorrect DPI access methods and presents a misleading example. It would be better to either remove or rewrite this snippet.