I'm thinking of a concurrent idea to go along with this to improve our own tests. See https://issues.apache.org/jira/browse/LOG4J2-653 for more details (not really, but at least an issue to track). I'm going to go back over a couple of the plugins I've converted and see how well the idea works out.
Also, I've been having strange issues on Jenkins. I had a build last over 24 hours (!) and still not get anywhere. I've also gotten INFRA to add us to Buildbot <http://ci.apache.org/builders/log4j2-nightly>, but the Java environment on there keeps causing certain tests to fail. See < https://issues.apache.org/jira/browse/INFRA-7671> for that. Then again, I could be mistaken in how JDK6 handles Long.parseLong("+42")? Also again, we're not getting any mail from Jenkins regarding builds. This has been part of the reason why I keep looking to alternative solutions considering how hard it is to set up Jenkins in the first place (from experience). On 25 May 2014 08:50, Remko Popma <[email protected]> wrote: > It could easily be that Matt ran all the tests and they all passed, but he > just forgot to commit the FlumeAppender file. That would explain the > compilation error. > > I'm not against the type conversion, please don't misunderstand. > About the @default annotations I'm still forming an opinion. I can see the > benefit, but something bothers me about the JUnit tests, still trying to > figure out what it is exactly that bothers me... > > When users run this code they will go through the PluginManager, and the > @default annotations will guarantee certain values. > In our previous mechanism to provide defaults, the JUnit tests could use > those defaults by specifying null. > With the new mechanism JUnit tests cannot use defaults. Tests need to > specify a value. So now it becomes easy to make a mistake in the JUnit > test. (The factory methods all have tons of parameters so they are already > easy to get wrong in the test...) > Also, we need to keep the JUnit test in sync when default values change > (before a JUnit test specifying null would automatically pick up the new > default). > > > > On Sun, May 25, 2014 at 10:24 PM, Gary Gregory <[email protected]>wrote: > >> The type converters still seems like a great idea. I've never liked >> having all of this boilerplate conversion code all over that place, even if >> most of it was refactored into calls to utils. It's too easy to make a >> mistake. >> >> WRT default values, then yes, we need to carefully consider what to do >> here, but since we are the primary customer, we can do whatever we want >> especially for the tests. >> >> Finger waving? ;-) sometimes a big pile of changes is safe, sometimes one >> character change is not... :-( What happened to running Log4J on a CI >> server? This would not solve this process issue but it would feedback an >> error to the ML and the committer ASAP. >> >> Gary >> >> >> On Sun, May 25, 2014 at 8:11 AM, Remko Popma <[email protected]>wrote: >> >>> I'm seeing a compile error in FlumeAppender (after the change >>> to Rfc5424Layout.createLayout). I fixed that, but now there are >>> few broken JUnit tests for the Flume appender: >>> >>> Tests in error: >>> FlumeAppenderTest.testBatch:237 ≫ IllegalArgument No structured id >>> name was su... >>> FlumeAppenderTest.testStructured:163 ≫ IllegalArgument No structured >>> id name w... >>> FlumeAppenderTest.testLog4jAvroAppender:134 ≫ IllegalArgument No >>> structured id... >>> FlumeAppenderTest.testNotConnected:301 ≫ IllegalArgument No structured >>> id name... >>> FlumeAppenderTest.testMultiple:203 ≫ IllegalArgument No structured id >>> name was... >>> FlumeAppenderTest.testReconnect:349 ≫ IllegalArgument No structured id >>> name wa... >>> FlumeAppenderTest.testConnectionRefused:271 ≫ IllegalArgument No >>> structured id... >>> >>> detail: >>> >>> testBatch(org.apache.logging.log4j.flume.appender.FlumeAppenderTest) >>> Time elapsed: 3.305 sec <<< ERROR! >>> java.lang.IllegalArgumentException: No structured id name was supplied >>> at >>> org.apache.logging.log4j.message.StructuredDataId.<init>(StructuredDataId.java:92) >>> at >>> org.apache.logging.log4j.core.layout.Rfc5424Layout.<init>(Rfc5424Layout.java:136) >>> at >>> org.apache.logging.log4j.core.layout.Rfc5424Layout.createLayout(Rfc5424Layout.java:652) >>> at >>> org.apache.logging.log4j.flume.appender.FlumeAppender.createAppender(FlumeAppender.java:223) >>> at >>> org.apache.logging.log4j.flume.appender.FlumeAppenderTest.testBatch(FlumeAppenderTest.java:237) >>> >>> >>> First, the mandatory finger wagging: please please run the tests before >>> committing, etc, etc. >>> >>> Second, I'm looking at how to fix these tests and now I'm not sure >>> whether the default annotation was a good idea: the thing is that the JUnit >>> tests use the factory methods directly but there is no PluginManager >>> involved, so nobody supplies the default value any more... >>> >>> That is why the JUnit tests above fail: FlumeAppender.createAppender() >>> passes a null argument to Rfc5424Layout.createLayout(), which is no longer >>> converted to the default value, which causes issues downstream. >>> >>> This raises the question: how do we deal with this? >>> >>> Should we change the FlumeAppender.createAppender() to provide a >>> non-null value? >>> That is possible of course, but then we only test the cases where a >>> value was supplied. Would we not be missing test cases this way? >>> >>> UPDATE: >>> I've tried providing a non-null value but the tests still fail. >>> >>> I want to work on other things now so I'm giving up on this. Matt, could >>> you take a look? >>> >>> Error detail: >>> testBatch(org.apache.logging.log4j.flume.appender.FlumeAppenderTest) >>> Time elapsed: 3.315 sec <<< ERROR! >>> org.apache.logging.log4j.core.appender.AppenderLoggingException: An >>> exception occurred processing Appender avro >>> at >>> org.apache.logging.log4j.core.net.Priority.getPriority(Priority.java:46) >>> at >>> org.apache.logging.log4j.core.layout.Rfc5424Layout.appendPriority(Rfc5424Layout.java:283) >>> at >>> org.apache.logging.log4j.core.layout.Rfc5424Layout.toSerializable(Rfc5424Layout.java:262) >>> at >>> org.apache.logging.log4j.core.layout.Rfc5424Layout.toSerializable(Rfc5424Layout.java:63) >>> at >>> org.apache.logging.log4j.core.layout.AbstractStringLayout.toByteArray(AbstractStringLayout.java:46) >>> at >>> org.apache.logging.log4j.flume.appender.FlumeAppender.append(FlumeAppender.java:102) >>> at >>> org.apache.logging.log4j.core.config.AppenderControl.callAppender(AppenderControl.java:97) >>> at >>> org.apache.logging.log4j.core.config.LoggerConfig.callAppenders(LoggerConfig.java:426) >>> at >>> org.apache.logging.log4j.core.config.LoggerConfig.log(LoggerConfig.java:405) >>> at >>> org.apache.logging.log4j.core.config.LoggerConfig.log(LoggerConfig.java:365) >>> at >>> org.apache.logging.log4j.core.Logger.logMessage(Logger.java:103) >>> at >>> org.apache.logging.log4j.spi.AbstractLoggerProvider.logMessage(AbstractLoggerProvider.java:1336) >>> at >>> org.apache.logging.log4j.spi.AbstractLoggerProvider.logIfEnabled(AbstractLoggerProvider.java:1325) >>> at org.apache.logging.log4j.spi.AbstractLoggerProvider.info >>> (AbstractLoggerProvider.java:907) >>> at >>> org.apache.logging.log4j.flume.appender.FlumeAppenderTest.testBatch(FlumeAppenderTest.java:248) >>> >> >> >> >> -- >> E-Mail: [email protected] | [email protected] >> Java Persistence with Hibernate, Second >> Edition<http://www.manning.com/bauer3/> >> JUnit in Action, Second Edition <http://www.manning.com/tahchiev/> >> Spring Batch in Action <http://www.manning.com/templier/> >> Blog: http://garygregory.wordpress.com >> Home: http://garygregory.com/ >> Tweet! http://twitter.com/GaryGregory >> > > -- Matt Sicker <[email protected]>
