You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by GitBox <gi...@apache.org> on 2020/10/21 22:10:00 UTC
[GitHub] [maven] rfscholte opened a new pull request #386: [MNG-6999] Chained (consumer) XMLFilters can result in "floating" comments
rfscholte opened a new pull request #386:
URL: https://github.com/apache/maven/pull/386
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] eolivelli commented on a change in pull request #386: [MNG-6999] Chained (consumer) XMLFilters can result in "floating" comments
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #386:
URL: https://github.com/apache/maven/pull/386#discussion_r509952862
##########
File path: maven-core/src/test/java/org/apache/maven/internal/aether/ConsumerModelSourceTransformerTest.java
##########
@@ -0,0 +1,69 @@
+package org.apache.maven.internal.aether;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import java.io.InputStream;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+
+import org.apache.maven.model.Model;
+import org.apache.maven.model.building.TransformerContext;
+import org.junit.Test;
+import org.xmlunit.assertj.XmlAssert;
+
+public class ConsumerModelSourceTransformerTest
+{
+ private ConsumerModelSourceTransformer transformer = new ConsumerModelSourceTransformer();
+
+ @Test
+ public void test() throws Exception
+ {
+ Path beforePomFile = Paths.get( "src/test/resources/projects/transform/before.pom").toAbsolutePath();
+ Path afterPomFile = Paths.get( "src/test/resources/projects/transform/after.pom").toAbsolutePath();
+
+ try( InputStream in = transformer.transform( beforePomFile, new NoTransformerContext() ) )
+ {
+ XmlAssert.assertThat( in ).and( Files.newInputStream( afterPomFile ) ).areIdentical();
Review comment:
are we closing the `Files.newInputStream( afterPomFile )` stream ?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] rfscholte commented on a change in pull request #386: [MNG-6999] Chained (consumer) XMLFilters can result in "floating" comments
Posted by GitBox <gi...@apache.org>.
rfscholte commented on a change in pull request #386:
URL: https://github.com/apache/maven/pull/386#discussion_r510301491
##########
File path: maven-xml/src/test/java/org/apache/maven/xml/sax/filter/ConsumerPomXMLFilterTest.java
##########
@@ -231,5 +234,22 @@ public void licenseHeader() throws Exception {
String actual = transform( input );
assertThat( actual ).and( expected ).areIdentical();
}
+
+ @Test
+// @Ignore
+ public void lexicalHandler() throws Exception
Review comment:
It tests that the lexical handler is used in both SaxSource and StreamResult. If one of both doesn't provide the lexical handler, all comments will be ignored. By default the SaxSource has no LexicalHandler, so the XMLParser will simply get elements, attributes, etc.
MNG-6999 exposed invalid chaining of lexical handlers, exposed by an IT, so I used that as an example. The test for removal of the comments is in the ModulesXMLFilterTest#comment
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] rfscholte commented on a change in pull request #386: [MNG-6999] Chained (consumer) XMLFilters can result in "floating" comments
Posted by GitBox <gi...@apache.org>.
rfscholte commented on a change in pull request #386:
URL: https://github.com/apache/maven/pull/386#discussion_r510301491
##########
File path: maven-xml/src/test/java/org/apache/maven/xml/sax/filter/ConsumerPomXMLFilterTest.java
##########
@@ -231,5 +234,22 @@ public void licenseHeader() throws Exception {
String actual = transform( input );
assertThat( actual ).and( expected ).areIdentical();
}
+
+ @Test
+// @Ignore
+ public void lexicalHandler() throws Exception
Review comment:
It tests that the lexical handler is used in both SaxSource and StreamResult. If one of both doesn't provide the lexical handler, all comments will be ignored. By default the SaxSource has no LexicalHandler, so the XMLParser will simply get elements, attributes, etc.
But yes, this is now testing 2 different things. I will add a test with a minimal pom + license, that should be enough to verify if the lexicalHander is properly chained.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] mthmulders commented on a change in pull request #386: [MNG-6999] Chained (consumer) XMLFilters can result in "floating" comments
Posted by GitBox <gi...@apache.org>.
mthmulders commented on a change in pull request #386:
URL: https://github.com/apache/maven/pull/386#discussion_r509948995
##########
File path: maven-core/src/test/java/org/apache/maven/internal/aether/ConsumerModelSourceTransformerTest.java
##########
@@ -0,0 +1,69 @@
+package org.apache.maven.internal.aether;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import java.io.InputStream;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+
+import org.apache.maven.model.Model;
+import org.apache.maven.model.building.TransformerContext;
+import org.junit.Test;
+import org.xmlunit.assertj.XmlAssert;
+
+public class ConsumerModelSourceTransformerTest
+{
+ private ConsumerModelSourceTransformer transformer = new ConsumerModelSourceTransformer();
+
+ @Test
+ public void test() throws Exception
Review comment:
Would it be possible to come up with a name for the test that better describes what it verifies?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] rfscholte commented on a change in pull request #386: [MNG-6999] Chained (consumer) XMLFilters can result in "floating" comments
Posted by GitBox <gi...@apache.org>.
rfscholte commented on a change in pull request #386:
URL: https://github.com/apache/maven/pull/386#discussion_r514956563
##########
File path: maven-core/src/test/java/org/apache/maven/project/ProjectBuilderTest.java
##########
@@ -249,9 +249,9 @@ public void testReadInvalidPom()
catch ( ProjectBuildingException ex )
{
assertEquals( 1, ex.getResults().size() );
- MavenProject project = ex.getResults().get( 0 ).getProject();
- assertNotNull( project );
- assertNotSame( 0, ex.getResults().get( 0 ).getProblems().size() );
+ assertNotNull( ex.getResults().get( 0 ).getPomFile() );
Review comment:
Noted. Now that we are using Java 8 it should be quite to catch these leaks by using assertThrows. If you hit such a test class, just fix it as part of continuous maintenance.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] rfscholte commented on a change in pull request #386: [MNG-6999] Chained (consumer) XMLFilters can result in "floating" comments
Posted by GitBox <gi...@apache.org>.
rfscholte commented on a change in pull request #386:
URL: https://github.com/apache/maven/pull/386#discussion_r510306319
##########
File path: maven-core/src/test/java/org/apache/maven/internal/aether/ConsumerModelSourceTransformerTest.java
##########
@@ -0,0 +1,69 @@
+package org.apache.maven.internal.aether;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import java.io.InputStream;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+
+import org.apache.maven.model.Model;
+import org.apache.maven.model.building.TransformerContext;
+import org.junit.Test;
+import org.xmlunit.assertj.XmlAssert;
+
+public class ConsumerModelSourceTransformerTest
+{
+ private ConsumerModelSourceTransformer transformer = new ConsumerModelSourceTransformer();
+
+ @Test
+ public void test() throws Exception
Review comment:
of course, my bad practice when starting a new class with only one method, to simply start with `test`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] rfscholte commented on pull request #386: [MNG-6999] Chained (consumer) XMLFilters can result in "floating" comments
Posted by GitBox <gi...@apache.org>.
rfscholte commented on pull request #386:
URL: https://github.com/apache/maven/pull/386#issuecomment-718921546
if there are no objections, I'll merge this tomorrow
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] mthmulders commented on a change in pull request #386: [MNG-6999] Chained (consumer) XMLFilters can result in "floating" comments
Posted by GitBox <gi...@apache.org>.
mthmulders commented on a change in pull request #386:
URL: https://github.com/apache/maven/pull/386#discussion_r509957484
##########
File path: maven-xml/src/test/java/org/apache/maven/xml/sax/filter/ConsumerPomXMLFilterTest.java
##########
@@ -231,5 +234,22 @@ public void licenseHeader() throws Exception {
String actual = transform( input );
assertThat( actual ).and( expected ).areIdentical();
}
+
+ @Test
+// @Ignore
Review comment:
I think this comment can be removed :-)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] rfscholte commented on a change in pull request #386: [MNG-6999] Chained (consumer) XMLFilters can result in "floating" comments
Posted by GitBox <gi...@apache.org>.
rfscholte commented on a change in pull request #386:
URL: https://github.com/apache/maven/pull/386#discussion_r510306596
##########
File path: maven-core/src/test/java/org/apache/maven/internal/aether/ConsumerModelSourceTransformerTest.java
##########
@@ -0,0 +1,69 @@
+package org.apache.maven.internal.aether;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import java.io.InputStream;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+
+import org.apache.maven.model.Model;
+import org.apache.maven.model.building.TransformerContext;
+import org.junit.Test;
+import org.xmlunit.assertj.XmlAssert;
+
+public class ConsumerModelSourceTransformerTest
+{
+ private ConsumerModelSourceTransformer transformer = new ConsumerModelSourceTransformer();
+
+ @Test
+ public void test() throws Exception
+ {
+ Path beforePomFile = Paths.get( "src/test/resources/projects/transform/before.pom").toAbsolutePath();
+ Path afterPomFile = Paths.get( "src/test/resources/projects/transform/after.pom").toAbsolutePath();
+
+ try( InputStream in = transformer.transform( beforePomFile, new NoTransformerContext() ) )
+ {
+ XmlAssert.assertThat( in ).and( Files.newInputStream( afterPomFile ) ).areIdentical();
Review comment:
no it does, good find!
##########
File path: maven-core/src/test/java/org/apache/maven/internal/aether/ConsumerModelSourceTransformerTest.java
##########
@@ -0,0 +1,69 @@
+package org.apache.maven.internal.aether;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import java.io.InputStream;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+
+import org.apache.maven.model.Model;
+import org.apache.maven.model.building.TransformerContext;
+import org.junit.Test;
+import org.xmlunit.assertj.XmlAssert;
+
+public class ConsumerModelSourceTransformerTest
+{
+ private ConsumerModelSourceTransformer transformer = new ConsumerModelSourceTransformer();
+
+ @Test
+ public void test() throws Exception
+ {
+ Path beforePomFile = Paths.get( "src/test/resources/projects/transform/before.pom").toAbsolutePath();
+ Path afterPomFile = Paths.get( "src/test/resources/projects/transform/after.pom").toAbsolutePath();
+
+ try( InputStream in = transformer.transform( beforePomFile, new NoTransformerContext() ) )
+ {
+ XmlAssert.assertThat( in ).and( Files.newInputStream( afterPomFile ) ).areIdentical();
Review comment:
now it does, good find!
##########
File path: maven-xml/src/test/java/org/apache/maven/xml/sax/filter/ConsumerPomXMLFilterTest.java
##########
@@ -231,5 +234,22 @@ public void licenseHeader() throws Exception {
String actual = transform( input );
assertThat( actual ).and( expected ).areIdentical();
}
+
+ @Test
+// @Ignore
Review comment:
true
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] MartinKanters commented on a change in pull request #386: [MNG-6999] Chained (consumer) XMLFilters can result in "floating" comments
Posted by GitBox <gi...@apache.org>.
MartinKanters commented on a change in pull request #386:
URL: https://github.com/apache/maven/pull/386#discussion_r514502330
##########
File path: maven-core/src/test/java/org/apache/maven/project/ProjectBuilderTest.java
##########
@@ -236,9 +236,9 @@ public void testReadInvalidPom()
{
projectBuilder.build( pomFile, configuration );
}
- catch ( InvalidArtifactRTException iarte )
+ catch ( Exception ex )
{
- assertThat( iarte.getMessage(), containsString( "The groupId cannot be empty." ) );
+ assertThat( ex.getMessage(), containsString( "expected START_TAG or END_TAG not TEXT" ) );
Review comment:
The extra space at the start can be removed I think.
##########
File path: maven-core/src/test/java/org/apache/maven/project/ProjectBuilderTest.java
##########
@@ -249,9 +249,9 @@ public void testReadInvalidPom()
catch ( ProjectBuildingException ex )
{
assertEquals( 1, ex.getResults().size() );
- MavenProject project = ex.getResults().get( 0 ).getProject();
- assertNotNull( project );
- assertNotSame( 0, ex.getResults().get( 0 ).getProblems().size() );
+ assertNotNull( ex.getResults().get( 0 ).getPomFile() );
Review comment:
It's not related to your merge request, but good practice anyway I think.
To make sure we get in these catch block we should add a `fail("Expected an exception to be thrown")` in the try-block after. I've had tests pass without ever reaching the catch 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] mthmulders commented on a change in pull request #386: [MNG-6999] Chained (consumer) XMLFilters can result in "floating" comments
Posted by GitBox <gi...@apache.org>.
mthmulders commented on a change in pull request #386:
URL: https://github.com/apache/maven/pull/386#discussion_r509958205
##########
File path: maven-xml/src/test/java/org/apache/maven/xml/sax/filter/ConsumerPomXMLFilterTest.java
##########
@@ -231,5 +234,22 @@ public void licenseHeader() throws Exception {
String actual = transform( input );
assertThat( actual ).and( expected ).areIdentical();
}
+
+ @Test
+// @Ignore
+ public void lexicalHandler() throws Exception
Review comment:
Should this test method not describe what it verifies, i.e. "removes modules" or something like that?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org