Use destination URI when creating resource during copy refactoring#3588
Use destination URI when creating resource during copy refactoring#3588mx990 wants to merge 2 commits intoeclipse-xtext:mainfrom
Conversation
Test Results 8 050 files - 22 8 050 suites - 22 3h 48m 6s ⏱️ - 20m 46s For more details on these failures, see this check. Results for commit 5a9216d. ± Comparison against base commit 088392b. ♻️ This comment has been updated with latest results. |
|
Wonder if this can somehow be tested |
|
@mx990 just for confirmation: are the added tests FAIL without your patch? |
|
Yes, (most of) the added tests fail without my patch when running them locally against main. |
mx990
force-pushed
the
refactoring-copy-uri-fix
branch
from
a69c9c0 to
d083cf6
Compare
....xtext.ui.tests/src-longrunning/org/eclipse/xtext/ui/tests/refactoring/ResourceCopyTest.java
Outdated
Show resolved
Hide resolved
....xtext.ui.tests/src-longrunning/org/eclipse/xtext/ui/tests/refactoring/ResourceCopyTest.java
Show resolved
Hide resolved
mx990
force-pushed
the
refactoring-copy-uri-fix
branch
from
d083cf6 to
f871b91
Compare
| import org.eclipse.core.resources.IResource; | ||
| import org.eclipse.jdt.core.IJavaElement; | ||
| import org.eclipse.jdt.internal.corext.refactoring.reorg.IReorgPolicy.ICopyPolicy; | ||
| import org.eclipse.jdt.internal.corext.refactoring.reorg.ReorgDestinationFactory; |
There was a problem hiding this comment.
hmm internal imports ...
There was a problem hiding this comment.
Likely dep in manifest also missing.
There was a problem hiding this comment.
hmm internal imports ...
There is, unfortunetaly, no generic copy processor for resources, so the test currently uses internal classes from JDT for that purpose. I could also create a mock copy processor to avoid the internal imports?
Likely dep in manifest also missing.
I have added the missing dependency for org.eclipse.jdt.core.manipulation in the manifest.
There was a problem hiding this comment.
@szarnekow @LorenzoBettini i really dislike the new big bunch of internal deps but also have no better idea
for a more isolated tests we seem to miss a ton of infra in ide.tests
There was a problem hiding this comment.
For tests it might acceptable. Otherwise, he suggested to use a mock avoiding the internal stuff
| * | ||
| * SPDX-License-Identifier: EPL-2.0 | ||
| */ | ||
| package org.eclipse.xtext.ui.tests.refactoring; |
There was a problem hiding this comment.
wonder if this could me moved to ide tests.
There was a problem hiding this comment.
I have tried to move the following related classes to org.eclipse.xtext.ide.tests:
AbstractResourceRelocationTest,ProgressReportingTest,ResourceCopyTest,ResourceMoveTest,ResourceRenameTest,
but this would require the following additional dependencies:
org.eclipse.core.resources,org.eclipse.jdt.core,org.eclipse.jdt.core.manipulation,org.eclipse.ltk.core.refactoring,org.eclipse.xtext.testlanguages.ui,org.eclipse.xtext.ui.testing.
Would you still like me to move the classes and add the additional dependencies?
There was a problem hiding this comment.
i thought based on a lsp test or xtext.ide.tests this could be done without any of these dependencies....
There was a problem hiding this comment.
(e.g. renameTestLanguage)
There was a problem hiding this comment.
I had a quick look at the lsp server implementation and Xtext does not seem to support any copy resource refactoring via lsp, since it handles neither willCreateFiles nor didCreateFiles from org.eclipse.lsp4j.services.WorkspaceService?
There was a problem hiding this comment.
hmmmm. thats bad. is there really no way to avoid the new internal deps?
There was a problem hiding this comment.
As mentioned above, I could also create a mock copy processor to avoid the internal deps?
When performing a COPY refactoring, the ResourceRelocationContext created the resource with the original URI and then changed the URI of the resource after loading. This left any proxy URIs in the loaded resource still pointing to the original file. When later resolving the proxies, a second resource for the original file was created on demand and any local references were resolved into that resource and not the copy. This avoids the problem by creating the resource with the destination URI instead, so that any proxies will be created with the correct URI.
This adds tests for the copy resource refactoring based on the Java copy processor.
mx990
force-pushed
the
refactoring-copy-uri-fix
branch
from
f871b91 to
5a9216d
Compare
|
can you also rebase your pr |
it is already on top of current main 088392b |
3 participants
When performing a
COPYrefactoring, theResourceRelocationContextcreated the resource with the original URI and then changed the URI of the resource after loading. This left any proxy URIs in the loaded resource still pointing to the original file. When later resolving the proxies, a second resource for the original file was created on demand and any local references were resolved into that resource and not the copy.This avoids the problem by creating the resource with the destination URI instead, so that any proxies will be created with the correct URI.
Fixes #3587