Re: [PR] [MSHARED-1290] Fix PropertyUtils cycle detection results in false positives [maven-filtering]

[via GitHub]

elharo merged PR #78:
URL: https://github.com/apache/maven-filtering/pull/78


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] [MSHARED-1290] Fix PropertyUtils cycle detection results in false positives [maven-filtering]

[via GitHub]

elharo commented on code in PR #78:
URL: https://github.com/apache/maven-filtering/pull/78#discussion_r1368496934


##
src/test/java/org/apache/maven/shared/filtering/PropertyUtilsTest.java:
##
@@ -39,40 +38,40 @@ public class PropertyUtilsTest extends TestSupport {
 private static File testDirectory = new File(getBasedir(), 
"target/test-classes/");
 
 public void testBasic() throws Exception {
-File basicProp = new File(testDirectory, "basic.properties");
+File basicProp = File.createTempFile("basic", ".properties");
 
-if (basicProp.exists()) {
+try {
+try (Writer writer = new OutputStreamWriter(new 
FileOutputStream(basicProp), StandardCharsets.UTF_8)) {
+writer.write("ghost=${non_existent}\n");
+writer.write("key=${untat_na_damgo}\n");
+writer.write("untat_na_damgo=gani_man\n");
+writer.flush();
+}
+
+Logger logger = mock(Logger.class);
+Properties prop = PropertyUtils.loadPropertyFile(basicProp, false, 
false, logger);

Review Comment:
   prop --> properties



##
src/test/java/org/apache/maven/shared/filtering/PropertyUtilsTest.java:
##
@@ -39,40 +38,40 @@ public class PropertyUtilsTest extends TestSupport {
 private static File testDirectory = new File(getBasedir(), 
"target/test-classes/");
 
 public void testBasic() throws Exception {
-File basicProp = new File(testDirectory, "basic.properties");
+File basicProp = File.createTempFile("basic", ".properties");
 
-if (basicProp.exists()) {
+try {
+try (Writer writer = new OutputStreamWriter(new 
FileOutputStream(basicProp), StandardCharsets.UTF_8)) {
+writer.write("ghost=${non_existent}\n");
+writer.write("key=${untat_na_damgo}\n");
+writer.write("untat_na_damgo=gani_man\n");
+writer.flush();
+}
+
+Logger logger = mock(Logger.class);
+Properties prop = PropertyUtils.loadPropertyFile(basicProp, false, 
false, logger);
+assertTrue(prop.getProperty("key").equals("gani_man"));
+assertTrue(prop.getProperty("ghost").equals("${non_existent}"));
+} finally {
 basicProp.delete();
 }
-
-basicProp.createNewFile();
-try (FileWriter writer = new FileWriter(basicProp)) {
-writer.write("ghost=${non_existent}\n");
-writer.write("key=${untat_na_damgo}\n");
-writer.write("untat_na_damgo=gani_man\n");
-writer.flush();
-}
-
-Properties prop = PropertyUtils.loadPropertyFile(basicProp, false, 
false);
-assertTrue(prop.getProperty("key").equals("gani_man"));
-assertTrue(prop.getProperty("ghost").equals("${non_existent}"));
 }
 
 public void testSystemProperties() throws Exception {
-File systemProp = new File(testDirectory, "system.properties");
+File systemProp = File.createTempFile("system", ".properties");
 
-if (systemProp.exists()) {
+try {
+try (Writer writer = new OutputStreamWriter(new 
FileOutputStream(systemProp), StandardCharsets.UTF_8)) {
+writer.write("key=${user.dir}");
+writer.flush();
+}
+
+Logger logger = mock(Logger.class);
+Properties prop = PropertyUtils.loadPropertyFile(systemProp, 
false, true, logger);
+
assertTrue(prop.getProperty("key").equals(System.getProperty("user.dir")));

Review Comment:
   assertEquals



##
src/test/java/org/apache/maven/shared/filtering/PropertyUtilsTest.java:
##
@@ -39,40 +38,40 @@ public class PropertyUtilsTest extends TestSupport {
 private static File testDirectory = new File(getBasedir(), 
"target/test-classes/");
 
 public void testBasic() throws Exception {
-File basicProp = new File(testDirectory, "basic.properties");
+File basicProp = File.createTempFile("basic", ".properties");
 
-if (basicProp.exists()) {
+try {
+try (Writer writer = new OutputStreamWriter(new 
FileOutputStream(basicProp), StandardCharsets.UTF_8)) {
+writer.write("ghost=${non_existent}\n");
+writer.write("key=${untat_na_damgo}\n");
+writer.write("untat_na_damgo=gani_man\n");
+writer.flush();
+}
+
+Logger logger = mock(Logger.class);
+Properties prop = PropertyUtils.loadPropertyFile(basicProp, false, 
false, logger);
+assertTrue(prop.getProperty("key").equals("gani_man"));

Review Comment:
   assertEquals



##
src/test/java/org/apache/maven/shared/filtering/PropertyUtilsTest.java:
##
@@ -39,40 +38,40 @@ public class PropertyUtilsTest extends TestSupport {
 private static File testDirectory = new File(getBasedir(), 
"target/test-classes/");
 

Re: [PR] [MSHARED-1290] Fix PropertyUtils cycle detection results in false positives [maven-filtering]

[via GitHub]

elharo commented on code in PR #78:
URL: https://github.com/apache/maven-filtering/pull/78#discussion_r1367913545


##
src/test/java/org/apache/maven/shared/filtering/PropertyUtilsTest.java:
##
@@ -143,32 +140,59 @@ public void testCircularReferences3Vars() throws 
IOException {
 }
 
 basicProp.createNewFile();
-try (FileWriter writer = new FileWriter(basicProp)) {
+try (Writer writer = new OutputStreamWriter(new 
FileOutputStream(basicProp), StandardCharsets.UTF_8)) {
 writer.write("test=${test2}\n");
 writer.write("test2=${test3}\n");
 writer.write("test3=${test}\n");
 writer.flush();
 }
 
 Logger logger = mock(Logger.class);
-
 Properties prop = PropertyUtils.loadPropertyFile(basicProp, null, 
logger);
 assertEquals("${test2}", prop.getProperty("test"));
 assertEquals("${test3}", prop.getProperty("test2"));
 assertEquals("${test}", prop.getProperty("test3"));
-assertWarn(
-logger,
-"Circular reference between properties detected: test3 => test 
=> test2 => test3",
-"Circular reference between properties detected: test2 => 
test3 => test => test2",
-"Circular reference between properties detected: test => test2 
=> test3 => test");
 }
 
-private void assertWarn(Logger mock, String... expected) {
-ArgumentCaptor argument = 
ArgumentCaptor.forClass(String.class);
-verify(mock, times(expected.length)).warn(argument.capture());
-List messages = argument.getAllValues();
-for (String str : expected) {
-assertTrue(messages.contains(str));
+public void testNonCircularReferences1Var3Times() throws IOException {
+File basicProp = new File(testDirectory, "non-circular.properties");

Review Comment:
   use File.createTempFile and delete it in a finally block



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] [MSHARED-1290] Fix PropertyUtils cycle detection results in false positives [maven-filtering]

[via GitHub]

wborn commented on PR #78:
URL: https://github.com/apache/maven-filtering/pull/78#issuecomment-1774095856

   Thanks for having a look! I've addressed your review comments in the new 
commit @elharo.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] [MSHARED-1290] Fix PropertyUtils cycle detection results in false positives [maven-filtering]

[via GitHub]

elharo commented on code in PR #78:
URL: https://github.com/apache/maven-filtering/pull/78#discussion_r1367902013


##
src/test/java/org/apache/maven/shared/filtering/PropertyUtilsTest.java:
##
@@ -171,4 +176,48 @@ private void assertWarn(Logger mock, String... expected) {
 assertTrue(messages.contains(str));
 }
 }
+
+public void testNonCircularReferences1Var3Times() throws IOException {
+File basicProp = new File(testDirectory, "non-circular.properties");
+
+if (basicProp.exists()) {
+basicProp.delete();
+}
+
+basicProp.createNewFile();
+try (FileWriter writer = new FileWriter(basicProp)) {

Review Comment:
   try to avoid assuming the default character encoding; e.g. by using an 
OutputStreamWriter directly inztead



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org