Skip to content

Use TestUtil.cleanUp across the tests #3029

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 7, 2025

Conversation

merks
Copy link
Contributor

@merks merks commented Jun 7, 2025

  • This ensures that the tests don't leak jobs that are only detected by a subsequent test.
  • Use editor.close(false) rather than editor.dispose() to clean up the thread/job properly.

#3025

merks and others added 2 commits June 7, 2025 11:57
- This ensures that the tests don't leak jobs that are only detected by
a subsequent test.
- Use editor.close(false) rather than editor.dispose() to clean up the
thread/job properly.

eclipse-platform#3025
@eclipse-platform-bot
Copy link
Contributor

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.ui.editors.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 abbe53ba6095b1758d00dfaa6320b816cf460cf6 Mon Sep 17 00:00:00 2001
From: Eclipse Platform Bot <[email protected]>
Date: Sat, 7 Jun 2025 10:02:33 +0000
Subject: [PATCH] Version bump(s) for 4.37 stream


diff --git a/tests/org.eclipse.ui.editors.tests/META-INF/MANIFEST.MF b/tests/org.eclipse.ui.editors.tests/META-INF/MANIFEST.MF
index 562aa8dcd7..58e2df3058 100644
--- a/tests/org.eclipse.ui.editors.tests/META-INF/MANIFEST.MF
+++ b/tests/org.eclipse.ui.editors.tests/META-INF/MANIFEST.MF
@@ -2,7 +2,7 @@ Manifest-Version: 1.0
 Bundle-ManifestVersion: 2
 Bundle-Name: %Plugin.name
 Bundle-SymbolicName: org.eclipse.ui.editors.tests;singleton:=true
-Bundle-Version: 3.13.800.qualifier
+Bundle-Version: 3.13.900.qualifier
 Bundle-Vendor: %Plugin.providerName
 Bundle-Localization: plugin
 Export-Package: org.eclipse.jface.text.tests.codemining,
-- 
2.49.0

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

Copy link
Contributor

github-actions bot commented Jun 7, 2025

Test Results

 2 778 files  ±0   2 778 suites  ±0   1h 35m 0s ⏱️ - 6m 33s
 7 925 tests ±0   7 697 ✅ ±0  228 💤 ±0  0 ❌ ±0 
23 324 runs  ±0  22 578 ✅ ±0  746 💤 ±0  0 ❌ ±0 

Results for commit c007c25. ± Comparison against base commit a71bf2c.

@merks merks merged commit f6c723f into eclipse-platform:master Jun 7, 2025
16 of 18 checks passed
@merks merks deleted the pr-fix-editor-tests branch June 7, 2025 10:49
@HannesWell
Copy link
Member

In some cases it might be more convenient to create for JUnit-4 tests a corresponding TestRule or for JUnit-5 a corresponding Extension.
The former is for example done in PDE at
https://github.com/eclipse-pde/eclipse.pde/blob/77b9fa8f8d2e8321976465ada201a69b01c5bcfb/ui/org.eclipse.pde.ui.tests/src/org/eclipse/pde/ui/tests/util/ProjectUtils.java#L308-L312

which is then for example used at:
https://github.com/eclipse-pde/eclipse.pde/blob/77b9fa8f8d2e8321976465ada201a69b01c5bcfb/ui/org.eclipse.pde.ui.tests/src/org/eclipse/pde/ui/tests/launcher/AbstractLaunchTest.java#L56

We also already have custom JUnit-5 extension here, but I don't remember from the top of my head where.

@merks
Copy link
Contributor Author

merks commented Jun 8, 2025

Yes, this seemed ugly and repetitive. But I'm not at JUnit expert though even to my eye it seemed there must be a better way.

vogella added a commit to vogella/eclipse.platform.ui that referenced this pull request Jun 8, 2025
In eclipse-platform#3029 it
was suggested to add a rule, this is the rule. It still need to be
applied to the test

TODO: Apply this to test
vogella added a commit to vogella/eclipse.platform.ui that referenced this pull request Jun 8, 2025
In eclipse-platform#3029 it
was suggested to add a rule, this is the rule. It still need to be
applied to the test

TODO: Apply this to test
@vogella
Copy link
Contributor

vogella commented Jun 8, 2025

Rule created #3031 here, let me know if you like that, if something thinks that is a good approach, I can modify the tests to use this rule.

@merks
Copy link
Contributor Author

merks commented Jun 8, 2025

Rule created #3031 here, let me know if you like that, if something thinks that is a good approach, I can modify the tests to use this rule.

I like it in principle a lot. I would expect that it should simplify something by virtue of deleting something.

Thanks 🙏 in advance for improving this.

vogella added a commit to vogella/eclipse.platform.ui that referenced this pull request Jun 10, 2025
In eclipse-platform#3029 it
was suggested to add a rule, this is the rule. It still need to be
applied to the test

TODO: Apply this to test
vogella added a commit to vogella/eclipse.platform.ui that referenced this pull request Jun 10, 2025
In eclipse-platform#3029 it
was suggested to add a rule, this is the rule. It is applied to the
tests.

TestRule is still necessary, as some tests spin the event loop or wait
for jobs using it.

Some tearDown methods did also spin the event loop, that is not
necessary as the rule does this already.
vogella added a commit to vogella/eclipse.platform.ui that referenced this pull request Jun 11, 2025
In eclipse-platform#3029 it
was suggested to add a rule, this is the rule. It is applied to the
tests.

TestRule is still necessary, as some tests spin the event loop or wait
for jobs using it.

Some tearDown methods did also spin the event loop, that is not
necessary as the rule does this already.
vogella added a commit to vogella/eclipse.platform.ui that referenced this pull request Jun 12, 2025
In eclipse-platform#3029 it
was suggested to add a rule, this is the rule. It is applied to the
tests.

Some tearDown methods did also spin the event loop, that is not
necessary as the rule does this already.
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.

4 participants