Skip to content

Conversation

aloowood
Copy link

No description provided.

Copy link
Contributor

github-actions bot commented Aug 16, 2025

Test Results

 2 781 files  ±0   2 781 suites  ±0   1h 52m 21s ⏱️ + 4m 31s
 7 931 tests ±0   7 704 ✅ +2  227 💤 ±0  0 ❌  - 1 
23 336 runs  ±0  22 603 ✅ +2  733 💤 ±0  0 ❌  - 1 

Results for commit d5b8e53. ± Comparison against base commit a7dc486.

♻️ This comment has been updated with latest results.

Copy link
Contributor

@merks merks left a comment

Choose a reason for hiding this comment

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

Hopefully it's possible to use IWorkbenchWindow and IWorkbenchPage instead of non-API implementation classes.

In general, please keep the PR to a single commit. Using amend and force push for that purpose rather than keep adding commits.

@aloowood aloowood requested a review from merks August 17, 2025 07:00
@aloowood aloowood marked this pull request as draft August 17, 2025 07:07
@aloowood aloowood marked this pull request as ready for review August 17, 2025 08:25
@aloowood aloowood force-pushed the dnd-tests branch 4 times, most recently from 7470cf0 to 05b8b4f Compare August 20, 2025 13:05
@aloowood
Copy link
Author

I have applied the requested changes, cleaned up all warnings, and rebased on top of the latest master. Ready for another review.

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.

Thank you for the update. Before we can take a deeper look into the proposal, there are some general concerns to address.

* This method should eventually replace the original one once the Workbench has been updated
* to handle Views and Editors without distincton.
*/
public static void drag(IWorkbenchPart part, TestDropLocation target, boolean wholeFolder) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think the tests can really work of this essential method still fails, do they?

import junit.framework.Test;

@RunWith(AllTests.class)
@Ignore("DND support not implemented yet in E4")
Copy link
Contributor

Choose a reason for hiding this comment

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

You need to remove the @Ignore and should set the test suite as the class executed for the bundle in the build.properties like in the other test bundles, e.g.:

pom.model.property.testClass = org.eclipse.ui.tests.UiTestSuite

We finally also need to add a proper test.xml to have the tests executed in Jenkins.


@RunWith(AllTests.class)
@Ignore("DND support not implemented yet in E4")
public class DragTestSuite {
Copy link
Contributor

Choose a reason for hiding this comment

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

This pretty much looks like a JUnit-3-style TestSuite. Since we are about to migrate everything to JUnit 5 (I just migrated everything to JUnit 4), reintroducing these tests should also incorporate and up-to-date style of the test suite.

@aloowood
Copy link
Author

aloowood commented Sep 3, 2025

I incorporated the feedback, rebased onto the current master, and squashed everything into a single commit. Thanks in advance for taking another look.

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.

Thank you for the latest update. Unfortunately, there is still no proper test suite and no test executed at all.
The DragTestSuite class is not a proper test suite (neither JUnit 4 nor JUnit 5). It is just a plain JUnit 5 test class with a paramaterized test method that effectively does nothing but instantiating the DragTest class without executing anything at all.
You will also notice that when executing the tests each of them is executed in less than millisecond (at least on my system) which is unlikely for a UI test.

@HeikoKlare
Copy link
Contributor

One additional hint to ensure that you first come to a functional state of the tests before pinging us for review: make sure that the tests that you add/update are actually executed by testing it locally. Run the tests in your IDE and in case you are not familiar with how to set up JUnit 4/5 tests / test suites and are in doubt if it works, you can e.g. set a breakpoint in the actual test methods to ensure that they are really executed. You should also run the single test classes individually to see that they also work without running the complete test suite.

@aloowood aloowood marked this pull request as draft September 23, 2025 09:18
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.

3 participants