Why is org.eclipse.jdt.core.manipulation which has no relation to UI in eclipse.jdt.ui and not in eclipse.jdt.core?

[arcivanov]

Seems a peculiar place for it to be, considering things like JDTLS depend on it and they are headless by nature.

[jjohnstn]

There is a relation to jdt.ui. There is code from jdt.ui that is refactored into jdt.core.manipulation with jdt.ui code changed to use the jdt.core.manipulation code as much as possible to prevent dual maintenance. This refactoring is still on-going to provide existing jdt.ui functionality for use in jdt.ls if possible and when adding new JDT UI features such as quick-assists/quick-fixes we split the code so jdt.ls can access the new functionality (with subsequent changes to jdt.ls). jdt.core.manipulation has be to kept in sync with jdt.ui plug-ins including the test suite.

If jdt.core.manipulation was in JDT Core, then a refactoring or new feature or even possiby a fix would require changes in both the JDT Core and JDT UI repos. The JDT UI PR build does not build JDT Core and uses a latest build (usually nightly). This means that the JDT UI part of the change would have to wait until the next nightly build after the JDT Core change is merged to successfully build and test. The JDT Core PR build would not be able properly test new jdt.core.manipulation changes and could fail because the UI tests require changing. With the current setup, changes can be submitted and properly verified in a single JDT UI PR.

[arcivanov]

I'm sorry, I'm not following this: " The JDT Core PR build would not be able properly test new jdt.core.manipulation changes and could fail because the UI tests require changing. With the current setup, changes can be submitted and properly verified in a single JDT UI PR."

jdt.core.manipulation has no UI dependency. It requires no UI for testing it - I just checked required bundles.

There is code from jdt.ui that is refactored into jdt.core.manipulation with jdt.ui code changed to use the jdt.core.manipulation code as much as possible to prevent dual maintenance.

Right, make sense, which means that manipulation can now be in JDT Core (with UI-independent tests extracted along with it) and tested without the UI harness, while the JDT UI tests the integration with manipulation through the UI harness. Or are you saying the only tests that are available for manipulation require JDT UI test harness?

[jjohnstn]

I should correct myself. The JDT Core PR build won't run JDT UI tests so won't be aware that a sync problem exists with the UI testsuite. There are currently no jdt.core.manipulation-only tests in JDT - the code is tested as part of JDT UI testing. Without such a test-suite a jdt.core.manipulation change in a JDT Core PR will be done without really testing it. When the PR is merged because there are no errors, if the UI tests are out of sync, the nightly build will report failures when it runs them and someone will have to determine whether they should be ignored or not. At present, this does not happen because any JDT UI PR that changes jdt.core.manipulation must pass the JDT UI testsuite. All tests added/change test both the JDT UI code and the jdt.core.manipulation code (if it is used).

Even if there was a jdt.core.manipulation-only testsuite, the UI testsuite tests will still be run in nightly builds and JDT UI PRS and these tests will go through both jdt.core.manipulation and JDT UI code as they should. When a change is made in the common code in jdt.core.manipulation, the tests in jdt.ui,tests are changed either because something that fails now works, the expected output changes, or a new test is needed to verify the new code does what it is supposed to do.

Making a JDT UI PR that changes jdt.core.manipulation and jdt.ui would have to first push/merge a change in JDT Core and then wait until the next nightly build occurs to verify the JDT UI tests of the PR to merge. As a JDT UI developer who is currently making such changes, including refactorings to enable jdt.ls, I see this as an impediment. IMO, writing a separate test-suite from scratch would not be beneficial for the effort required since the code is already being tested by both JDT UI and jdt.ls test-suites and still requires new tests added after any changes are made.