From 650d7ea6803b26eb1299ae4423b5005f3c98d0d0 Mon Sep 17 00:00:00 2001 From: Titouan Vervack Date: Fri, 26 Apr 2024 11:50:19 +0200 Subject: [PATCH 1/4] Correctly update location attributes in Target Editor These were completely ignored before iar add test --- .../core/target/TargetDefinition.java | 8 ++++ .../tests/target/IUBundleContainerTests.java | 44 ++++++++++++++++++- 2 files changed, 50 insertions(+), 2 deletions(-) diff --git a/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/target/TargetDefinition.java b/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/target/TargetDefinition.java index 5b56dc08f1..5e9e3caf30 100644 --- a/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/target/TargetDefinition.java +++ b/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/target/TargetDefinition.java @@ -1290,6 +1290,8 @@ private void updateIUContainerElements(Element containersElement, List } } + // The containers that were only updated. Doesn't include brand-new ones + List updatedContainers = new ArrayList<>(newContainers); for (Element container : newContainers) { NodeList nodes = container.getChildNodes(); List units = new ArrayList<>(); @@ -1321,10 +1323,16 @@ private void updateIUContainerElements(Element containersElement, List } else { Node movedContainer = fDocument.importNode(container, true); TargetDefinitionDocumentTools.addChildWithIndent(containersElement, movedContainer); + updatedContainers.remove(container); } } } + // Use a mock comparator, we only want to use the + // "elementAttributesComparator" also used in updateElements + TargetDefinitionDocumentTools.updateElements(containersElement, oldContainers, updatedContainers, + (Element e1, Element e2) -> 0); + for (Entry> entry : oldContainersByRepo.entrySet()) { entry.getValue().forEach(TargetDefinitionDocumentTools::removeChildAndWhitespace); } diff --git a/ui/org.eclipse.pde.ui.tests/src/org/eclipse/pde/ui/tests/target/IUBundleContainerTests.java b/ui/org.eclipse.pde.ui.tests/src/org/eclipse/pde/ui/tests/target/IUBundleContainerTests.java index f23a2e374e..a48cbe9a99 100644 --- a/ui/org.eclipse.pde.ui.tests/src/org/eclipse/pde/ui/tests/target/IUBundleContainerTests.java +++ b/ui/org.eclipse.pde.ui.tests/src/org/eclipse/pde/ui/tests/target/IUBundleContainerTests.java @@ -15,6 +15,7 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotEquals; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertTrue; @@ -592,9 +593,10 @@ public void testSerialization2() throws Exception { @Test public void testSerialization3() throws Exception { URI uri = getURI("/tests/sites/site.a.b"); - String[] unitIds = new String[]{"feature.b.feature.group"}; + String[] unitIds = new String[] { "feature.b.feature.group" }; IInstallableUnit[] units = getUnits(unitIds, uri); - IUBundleContainer location = createContainer(units, new URI[] {uri}, IUBundleContainer.INCLUDE_ALL_ENVIRONMENTS | IUBundleContainer.INCLUDE_SOURCE); + IUBundleContainer location = createContainer(units, new URI[] { uri }, + IUBundleContainer.INCLUDE_ALL_ENVIRONMENTS | IUBundleContainer.INCLUDE_SOURCE); String xml = location.serialize(); assertIncludeAllPlatform(xml, true); assertIncludeMode(xml, "slicer"); @@ -602,6 +604,44 @@ public void testSerialization3() throws Exception { deserializationTest(location); } + @Test + public void testSerializationOnlyLocationAttributeChanged() throws Exception { + URI uri = getURI("/tests/sites/site.a.b"); + String[] unitIds = new String[]{"feature.b.feature.group"}; + IInstallableUnit[] units = getUnits(unitIds, uri); + + IUBundleContainer location1 = createContainer(units, new URI[] { uri }, + IUBundleContainer.INCLUDE_ALL_ENVIRONMENTS | IUBundleContainer.INCLUDE_SOURCE); + String xml1 = location1.serialize(); + assertIncludeAllPlatform(xml1, true); + assertIncludeMode(xml1, "slicer"); + assertIncludeSource(xml1, true); + + IUBundleContainer location2 = createContainer(units, new URI[] { uri }, + IUBundleContainer.INCLUDE_ALL_ENVIRONMENTS); // no source + String xml2 = location2.serialize(); + assertIncludeAllPlatform(xml2, true); + assertIncludeMode(xml2, "slicer"); + assertIncludeSource(xml2, false); // no source + + ITargetDefinition td = getTargetService().newTarget(); + td.setTargetLocations(new ITargetLocation[] { location1 }); + ByteArrayOutputStream out1 = new ByteArrayOutputStream(); + TargetDefinitionPersistenceHelper.persistXML(td, out1); + String resultXmlOld = new String(out1.toByteArray()); + + td.setTargetLocations(new ITargetLocation[] { location2 }); + ByteArrayOutputStream out2 = new ByteArrayOutputStream(); + TargetDefinitionPersistenceHelper.persistXML(td, out2); + String resultXmlNew = new String(out2.toByteArray()); + + String normalizedOld = resultXmlOld.replaceAll("\r?\n[ \t]*", ""); + String normalizedNew = resultXmlNew.replaceAll("\r?\n[ \t]*", ""); + + assertNotEquals(normalizedOld, normalizedNew); + assertEquals(normalizedOld, normalizedNew.replace("includeSource=\"false\"", "includeSource=\"true\"")); + } + public void deserializationTest(IUBundleContainer location) throws Exception { ITargetDefinition td = getTargetService().newTarget(); td.setTargetLocations(new ITargetLocation[] {location}); From cb8f641c4a211baf5708de73fd5d736bccde0c63 Mon Sep 17 00:00:00 2001 From: Titouan Vervack Date: Fri, 26 Apr 2024 11:50:56 +0200 Subject: [PATCH 2/4] Small cleanup Using a local variable and putIfAbsent makes the code more compact and legible --- .../internal/core/target/TargetDefinition.java | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/target/TargetDefinition.java b/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/target/TargetDefinition.java index 5e9e3caf30..f0448caf31 100644 --- a/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/target/TargetDefinition.java +++ b/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/target/TargetDefinition.java @@ -1270,15 +1270,12 @@ private void updateIUContainerElements(Element containersElement, List for (int i = 0; i < nodes.getLength(); i++) { Node node = nodes.item(i); if (node instanceof Element) { - if (repoURL == null - && node.getNodeName().equalsIgnoreCase(TargetDefinitionPersistenceHelper.REPOSITORY)) { + String nodeName = node.getNodeName(); + if (repoURL == null && nodeName.equalsIgnoreCase(TargetDefinitionPersistenceHelper.REPOSITORY)) { repoURL = ((Element) node).getAttribute(TargetDefinitionPersistenceHelper.LOCATION); - if (!oldContainersByRepo.containsKey(repoURL)) { - oldContainersByRepo.put(repoURL, new ArrayList<>()); - } + oldContainersByRepo.putIfAbsent(repoURL, new ArrayList<>()); oldContainersByRepo.get(repoURL).add(container); - } else if (node.getNodeName() - .equalsIgnoreCase(TargetDefinitionPersistenceHelper.INSTALLABLE_UNIT)) { + } else if (nodeName.equalsIgnoreCase(TargetDefinitionPersistenceHelper.INSTALLABLE_UNIT)) { units.add((Element) node); } } @@ -1299,11 +1296,10 @@ private void updateIUContainerElements(Element containersElement, List for (int i = 0; i < nodes.getLength(); i++) { Node node = nodes.item(i); if (node instanceof Element) { - if (repoURL == null - && node.getNodeName().equalsIgnoreCase(TargetDefinitionPersistenceHelper.REPOSITORY)) { + String nodeName = node.getNodeName(); + if (repoURL == null && nodeName.equalsIgnoreCase(TargetDefinitionPersistenceHelper.REPOSITORY)) { repoURL = ((Element) node).getAttribute(TargetDefinitionPersistenceHelper.LOCATION); - } else if (node.getNodeName() - .equalsIgnoreCase(TargetDefinitionPersistenceHelper.INSTALLABLE_UNIT)) { + } else if (nodeName.equalsIgnoreCase(TargetDefinitionPersistenceHelper.INSTALLABLE_UNIT)) { units.add((Element) node); } } From 78407cdee525f7cd8d1cb26ca623c13439036bc7 Mon Sep 17 00:00:00 2001 From: Titouan Vervack Date: Fri, 26 Apr 2024 15:04:31 +0200 Subject: [PATCH 3/4] Pass expected result as expected, not actual --- .../extension/tests/Bug531602FormattingTests.java | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/ui/org.eclipse.pde.genericeditor.extension.tests/src/org/eclipse/pde/genericeditor/extension/tests/Bug531602FormattingTests.java b/ui/org.eclipse.pde.genericeditor.extension.tests/src/org/eclipse/pde/genericeditor/extension/tests/Bug531602FormattingTests.java index 9a7282623a..960513be94 100644 --- a/ui/org.eclipse.pde.genericeditor.extension.tests/src/org/eclipse/pde/genericeditor/extension/tests/Bug531602FormattingTests.java +++ b/ui/org.eclipse.pde.genericeditor.extension.tests/src/org/eclipse/pde/genericeditor/extension/tests/Bug531602FormattingTests.java @@ -13,13 +13,13 @@ *******************************************************************************/ package org.eclipse.pde.genericeditor.extension.tests; +import static java.nio.charset.StandardCharsets.UTF_8; import static org.junit.Assert.assertEquals; import java.io.ByteArrayOutputStream; import java.io.IOException; import java.io.InputStream; import java.net.URI; -import java.nio.charset.StandardCharsets; import java.util.Arrays; import java.util.Scanner; @@ -50,8 +50,7 @@ public void testSettingNullPersists() throws Exception { ByteArrayOutputStream actualOutput = new ByteArrayOutputStream(); targetDefinition.setProgramArguments(null); TargetDefinitionPersistenceHelper.persistXML(targetDefinition, actualOutput); - assertEquals(expectedOutput.toString(StandardCharsets.UTF_8.toString()), - actualOutput.toString(StandardCharsets.UTF_8.toString())); + assertEquals(expectedOutput.toString(UTF_8.toString()), actualOutput.toString(UTF_8.toString())); } @Test @@ -105,9 +104,6 @@ public void testITargetLocationExtensionSerialization() throws Exception { confirmMatch(targetDefinition, "ITargetLocationExtensionTestCaseTarget.txt"); } - public static void assertEqualStringIgnoreDelim(String actual, String expected) throws IOException { - StringAsserts.assertEqualStringIgnoreDelim(actual, expected); - } private void confirmMatch(ITargetDefinition targetDefinition, String expectedDefinitionPath) throws Exception { try (Scanner s = new Scanner(FrameworkUtil.getBundle(this.getClass()) .getEntry("testing-files/target-files/" + expectedDefinitionPath).openStream()).useDelimiter("\\A")) { @@ -123,7 +119,7 @@ private void confirmMatch(ITargetDefinition targetDefinition, String expectedDef ByteArrayOutputStream actualOutput = new ByteArrayOutputStream(); TargetDefinitionPersistenceHelper.persistXML(targetDefinition, actualOutput); - assertEqualStringIgnoreDelim(result, actualOutput.toString(StandardCharsets.UTF_8.toString())); + StringAsserts.assertEqualStringIgnoreDelim(actualOutput.toString(UTF_8.toString()), result); } catch (IOException e) { } } From afdb8e58a37e3c62f9046d5008565bc9cd5fbdb4 Mon Sep 17 00:00:00 2001 From: Titouan Vervack Date: Thu, 2 May 2024 08:26:55 +0200 Subject: [PATCH 4/4] Apply suggestions from code review Co-authored-by: Hannes Wellmann --- .../eclipse/pde/internal/core/target/TargetDefinition.java | 2 +- .../extension/tests/Bug531602FormattingTests.java | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/target/TargetDefinition.java b/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/target/TargetDefinition.java index f0448caf31..61fd8719b0 100644 --- a/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/target/TargetDefinition.java +++ b/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/target/TargetDefinition.java @@ -1327,7 +1327,7 @@ private void updateIUContainerElements(Element containersElement, List // Use a mock comparator, we only want to use the // "elementAttributesComparator" also used in updateElements TargetDefinitionDocumentTools.updateElements(containersElement, oldContainers, updatedContainers, - (Element e1, Element e2) -> 0); + (e1, e2) -> 0); for (Entry> entry : oldContainersByRepo.entrySet()) { entry.getValue().forEach(TargetDefinitionDocumentTools::removeChildAndWhitespace); diff --git a/ui/org.eclipse.pde.genericeditor.extension.tests/src/org/eclipse/pde/genericeditor/extension/tests/Bug531602FormattingTests.java b/ui/org.eclipse.pde.genericeditor.extension.tests/src/org/eclipse/pde/genericeditor/extension/tests/Bug531602FormattingTests.java index 960513be94..e2b9b9d2fd 100644 --- a/ui/org.eclipse.pde.genericeditor.extension.tests/src/org/eclipse/pde/genericeditor/extension/tests/Bug531602FormattingTests.java +++ b/ui/org.eclipse.pde.genericeditor.extension.tests/src/org/eclipse/pde/genericeditor/extension/tests/Bug531602FormattingTests.java @@ -13,13 +13,13 @@ *******************************************************************************/ package org.eclipse.pde.genericeditor.extension.tests; -import static java.nio.charset.StandardCharsets.UTF_8; import static org.junit.Assert.assertEquals; import java.io.ByteArrayOutputStream; import java.io.IOException; import java.io.InputStream; import java.net.URI; +import java.nio.charset.StandardCharsets; import java.util.Arrays; import java.util.Scanner; @@ -50,7 +50,7 @@ public void testSettingNullPersists() throws Exception { ByteArrayOutputStream actualOutput = new ByteArrayOutputStream(); targetDefinition.setProgramArguments(null); TargetDefinitionPersistenceHelper.persistXML(targetDefinition, actualOutput); - assertEquals(expectedOutput.toString(UTF_8.toString()), actualOutput.toString(UTF_8.toString())); + assertEquals(expectedOutput.toString(StandardCharsets.UTF_8), actualOutput.toString(StandardCharsets.UTF_8)); } @Test @@ -119,7 +119,7 @@ private void confirmMatch(ITargetDefinition targetDefinition, String expectedDef ByteArrayOutputStream actualOutput = new ByteArrayOutputStream(); TargetDefinitionPersistenceHelper.persistXML(targetDefinition, actualOutput); - StringAsserts.assertEqualStringIgnoreDelim(actualOutput.toString(UTF_8.toString()), result); + StringAsserts.assertEqualStringIgnoreDelim(actualOutput.toString(StandardCharsets.UTF_8), result); } catch (IOException e) { } }