Skip to content

Check also device native zoom in Test_org_eclipse_swt_widgets_Shell - #3598

Closed
trancexpress wants to merge 1 commit into
eclipse-platform:masterfrom
trancexpress:gh3597
Closed

trancexpress wants to merge 1 commit into
eclipse-platform:masterfrom
trancexpress:gh3597

Conversation

@trancexpress

@trancexpress trancexpress commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

Some test methods in Test_org_eclipse_swt_widgets_Shell fail on Windows with an unexpected Shell size, where the difference is one less pixel for the Shell height.

This change broadens the tolerance of 1 pixel on Shell zoom levels different than 100 to apply also if DPIUtil.getNativeDeviceZoom() returns a value which is not 100.
The actual zoom value used for size computations is taken from Control.computeBoundsZoom(), which delegates to DPIUtil.getNativeDeviceZoom().

Fixes: #3597

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟢 Approval recommended

No unresolved issues were identified that would block approval.

Pull request overview

Adjusts Shell size assertions to tolerate a one-pixel discrepancy on Windows, preventing intermittent I-build failures.

Changes:

  • Uses the tolerance helper in both integer- and Point-based size tests.
  • Enables one-pixel tolerance for Windows shells.
File summaries
File Description
tests/org.eclipse.swt.tests/JUnit Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_widgets_Shell.java Updated as part of this pull request.
Review details
  • Files reviewed: 1/1 changed files
  • Comments generated: 0
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@trancexpress

Copy link
Copy Markdown
Contributor Author

@akurtakov WDYT here? Should we try to investigate the 1 pixel height difference, or just ignore it?

@akurtakov

Copy link
Copy Markdown
Member

It is always good to chase such things if someone has time for it. As it's Windows thing I'll defer to @HeikoKlare to have a say.

@github-actions

github-actions Bot commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

Test Results

  212 files  ±0    212 suites  ±0   26m 49s ⏱️ - 2m 7s
4 959 tests ±0  4 932 ✅ ±0   27 💤 ±0  0 ❌ ±0 
7 187 runs  ±0  6 996 ✅ ±0  191 💤 ±0  0 ❌ ±0 

Results for commit af39102. ± Comparison against base commit 57a42d9.

♻️ This comment has been updated with latest results.

@trancexpress

trancexpress commented Sep 17, 2026

Copy link
Copy Markdown
Contributor Author

One of the fails is at:

		newSize = new Point(1292, 1036);
		for (int i = 0; i < 10; i++) {
			testShell.setSize(newSize);
			assertShellProperlySized(testShell, newSize);
			newSize.x -= 100;
			newSize.y -= 100;
		}
Shell is not sized correctly (expected size: Point {792, 536} ± 0, actual size: Point {792, 535}) ==> expected: <true> but was: <false>

So zoom was 100 (no tolerance for the diff in assertShellProperlySized) and for several iterations the set heights were fine: 1036, 936, 836, 736, 636

The other fail looks more or less the same, aside from not calling assertShellProperlySized:

		newSize = new Point(1292, 1036);
		for (int i = 0; i < 10; i++) {
			testShell.setSize(newSize.x, newSize.y);
			assertEquals(newSize, testShell.getSize());
			newSize.x -= 100;
			newSize.y -= 100;
		}

Maybe Control.getAutoscalingZoom()/Control.computeBoundsZoom() didn't return 100, but Shell.getZoom() did? Those are different implementations.

How about trying this instead?

diff --git a/tests/org.eclipse.swt.tests/JUnit Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_widgets_Shell.java b/tests/org.eclipse.swt.tests/JUnit Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_widgets_Shell.java
index bebbe450e8..2a888b5d59 100644
--- a/tests/org.eclipse.swt.tests/JUnit Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_widgets_Shell.java       
+++ b/tests/org.eclipse.swt.tests/JUnit Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_widgets_Shell.java       
@@ -35,6 +35,7 @@ import org.eclipse.swt.events.SelectionAdapter;
 import org.eclipse.swt.events.SelectionEvent;
 import org.eclipse.swt.events.ShellEvent;
 import org.eclipse.swt.events.ShellListener;
+import org.eclipse.swt.graphics.AutoscalingMode;
 import org.eclipse.swt.graphics.Point;
 import org.eclipse.swt.graphics.Rectangle;
 import org.eclipse.swt.graphics.Region;
@@ -65,6 +66,14 @@ public void setUp() {
        testShell = new Shell(shell, SWT.NULL);
        setWidget(shell);
        assertEquals(shell, testShell.getParent());
+       /**
+        * Make sure we don't have unexpected zoom levels during the size calculations,
+        * since Shell.getZoom() and the zoom used for size calculations (Control.computeBoundsZoom()
+        * can potentially differ. See: https://github.com/eclipse-platform/eclipse.platform.swt/issues/3597
+        */
+       if (SwtTestUtil.isWindows && testShell.getZoom() == 100) {
+               testShell.setAutoscalingMode(AutoscalingMode.DISABLED);
+       }
 }
 
 @Test

I think the actual value used for size computations is DPIUtil.getNativeDeviceZoom(), so I've adjusted the assertion method to also check that value.

@trancexpress trancexpress changed the title Allow tolerance of 1 pixel when comparing Shell size in Test_org_eclipse_swt_widgets_Shell under Windows Check also device native zoom in Test_org_eclipse_swt_widgets_Shell Sep 17, 2026
Some test methods in Test_org_eclipse_swt_widgets_Shell fail on Windows with an unexpected Shell size,
where the difference is one less pixel for the Shell height.

This change broadens the tolerance of 1 pixel on Shell zoom levels different than 100
to apply also if DPIUtil.getNativeDeviceZoom() returns a value which is not 100.
The actual zoom value used for size computations is taken from Control.computeBoundsZoom(),
which delegates to DPIUtil.getNativeDeviceZoom().

Fixes: eclipse-platform#3597

@HeikoKlare HeikoKlare left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you for working on the test failure. The proposed fix may only solve a test failure that happens on a system with two monitors, one at 100% an one at a different zoom, and with another shell than the test shell being moved to a different monitor than the test shell is placed on while the assertion is evaluated (otherwise shell.getZoom() == DPIUtil.getNativeDeviceZoom()). I am not very confident that we have that situation in I-Builds. Actually, I expect our Jenkins Windows agent to just have a single-monitor setup with 100%.

With some analysis I found a bug in the actual business logic that may cause this issue. Even though not 100% sure if that is actually the cause, we should fix that bug anyway. And I propose to then wait for several I-Builds to see if the test failure happens again or if that fix has already resolved it:

@trancexpress

Copy link
Copy Markdown
Contributor Author

And I propose to then wait for several I-Builds to see if the test failure happens again or if that fix has already resolved it:

Alright, lets close this PR in favor of your fix. Hopefully the fails are gone, if not we can always re-open. Thank you for taking a look!

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.

Test_org_eclipse_swt_widgets_Shell failures in I-build

4 participants