Skip to content

Try to stabilize flacky PartRenderingEngineTest #3063

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

Merged
merged 2 commits into from
Jun 26, 2025

Conversation

ptziegler
Copy link
Contributor

@ptziegler ptziegler commented Jun 24, 2025

Due to an oversight, the REMOVE_ON_HIDE tag is set on "partB" twice, as opposed to "partB" and "partC". This may occasionally keep the part stack rendered, because "partC" is not removed from the model when hidden.

Closes #751

@ptziegler
Copy link
Contributor Author

This issue can hardly be reproduced locally, so I'll keep it as a draft for now and will periodically rebase it, to see whether the failure still occurs. The general problem is clear, I think, and the question is only how much one has to wait between the test steps.

@eclipse-platform-bot
Copy link
Contributor

eclipse-platform-bot commented Jun 24, 2025

This pull request changes some projects for the first time in this development cycle.
Therefore the following files need a version increment:

tests/org.eclipse.e4.ui.tests/META-INF/MANIFEST.MF

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 patch
From e301080b7e624b7923f0bf36116238e9a6e73c24 Mon Sep 17 00:00:00 2001
From: Eclipse Platform Bot <[email protected]>
Date: Thu, 26 Jun 2025 05:21:08 +0000
Subject: [PATCH] Version bump(s) for 4.37 stream


diff --git a/tests/org.eclipse.e4.ui.tests/META-INF/MANIFEST.MF b/tests/org.eclipse.e4.ui.tests/META-INF/MANIFEST.MF
index 0b2cce81a1..e977391e55 100644
--- a/tests/org.eclipse.e4.ui.tests/META-INF/MANIFEST.MF
+++ b/tests/org.eclipse.e4.ui.tests/META-INF/MANIFEST.MF
@@ -2,7 +2,7 @@ Manifest-Version: 1.0
 Bundle-ManifestVersion: 2
 Bundle-Name: %pluginName
 Bundle-SymbolicName: org.eclipse.e4.ui.tests;singleton:=true
-Bundle-Version: 0.15.800.qualifier
+Bundle-Version: 0.15.900.qualifier
 Bundle-Vendor: %providerName
 Bundle-Localization: plugin
 Require-Bundle: org.eclipse.emf.ecore.xmi;bundle-version="2.4.0",
-- 
2.49.0

Further information are available in Common Build Issues - Missing version increments.

Copy link
Contributor

github-actions bot commented Jun 24, 2025

Test Results

 2 778 files  ±0   2 778 suites  ±0   1h 39m 50s ⏱️ + 2m 22s
 7 929 tests ±0   7 700 ✅ ±0  228 💤 ±0  1 ❌ ±0 
23 340 runs  ±0  22 593 ✅ ±0  746 💤 ±0  1 ❌ ±0 

For more details on these failures, see this check.

Results for commit 98eb320. ± Comparison against base commit 51c4ffc.

♻️ This comment has been updated with latest results.

@@ -2468,6 +2468,7 @@ public void ensureCleanUpAddonCleansUp() {

assertTrue(" PartStack with children should be rendered", partStackForPartBPartC.isToBeRendered());
partService.hidePart(partB);
contextRule.spinEventLoop();
partService.hidePart(partC);
contextRule.spinEventLoop();
assertFalse(
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead one can consider to have a loop here that for example checks for a maximum of 10 seconds if the condition is meet and if not spins the event loop. I have did this on other places in a similar way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I first wanted to see if this is sufficient, but I was also already playing with the idea of creating a new spinEventLoop(int method), if this is still too unreliable:

public void spinEventLoop(int time) {
    long start = System.currentTimeMillis();
    do {
        while (Display.getCurrent().readAndDispatch()) {
            Thread.yield();
        }
    } while (System.currentTimeMillis() - start < time);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

False alarm: It's not a timing issue, the test is simply bugged. "partC" is missing the REMOVE_ON_HIDE_TAG, so it's never removed from the part stack.

@ptziegler ptziegler marked this pull request as ready for review June 26, 2025 05:12
Due to an oversight, the REMOVE_ON_HIDE tag is set on "partB" twice, as
opposed to "partB" and "partC". This may occasionally keep the part
stack rendered, because "partC" is not removed from the model when
hidden.

Closes eclipse-platform#751
@ptziegler
Copy link
Contributor Author

For the sake of documentation:

  • The failure can be reproduced by simply running the test in a loop. After ~80-100 cycles, the problem should occur. Cause of the failure is the CleanupAddon not being executed.
  • The test was added with a64d6c9 by @vogella and I'm fairly certain that this line was simply a copy-paste error.

@vogella
Copy link
Contributor

vogella commented Jun 26, 2025

For the sake of documentation:

  • The failure can be reproduced by simply running the test in a loop. After ~80-100 cycles, the problem should occur. Cause of the failure is the CleanupAddon not being executed.
  • The test was added with a64d6c9 by @vogella and I'm fairly certain that this line was simply a copy-paste error.

Using partC looks right.

@ptziegler
Copy link
Contributor Author

Test failure is unrelated: #1517

@ptziegler ptziegler merged commit 69d52f3 into eclipse-platform:master Jun 26, 2025
16 of 18 checks passed
@ptziegler ptziegler deleted the issue751 branch June 26, 2025 09:09
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.

PartRenderingEngineTests test fails randomly
4 participants