You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@logging.apache.org by Matt Sicker <bo...@gmail.com> on 2022/01/12 18:07:25 UTC
Re: [logging-log4j2] 02/04: Our convention is to make test classes public.
I'll note that the convention from JUnit 4 is to make them public;
JUnit 5 encourages package-private tests instead for some reason, and
that's the default template for JUnit 5 tests in IntelliJ. I do like
consistency, though!
On Wed, Jan 12, 2022 at 11:01 AM <gg...@apache.org> wrote:
>
> This is an automated email from the ASF dual-hosted git repository.
>
> ggregory pushed a commit to branch release-2.x
> in repository https://gitbox.apache.org/repos/asf/logging-log4j2.git
>
> commit b4a892acd1d37f355c27cde10ab1736cf1ebe315
> Author: Gary Gregory <ga...@gmail.com>
> AuthorDate: Wed Jan 12 11:58:30 2022 -0500
>
> Our convention is to make test classes public.
> ---
> .../test/java/org/apache/logging/log4j/core/net/SmtpManagerTest.java | 2 +-
> .../org/apache/logging/log4j/core/net/SocketAppenderReconnectTest.java | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/log4j-core/src/test/java/org/apache/logging/log4j/core/net/SmtpManagerTest.java b/log4j-core/src/test/java/org/apache/logging/log4j/core/net/SmtpManagerTest.java
> index df98702..5759cf7 100644
> --- a/log4j-core/src/test/java/org/apache/logging/log4j/core/net/SmtpManagerTest.java
> +++ b/log4j-core/src/test/java/org/apache/logging/log4j/core/net/SmtpManagerTest.java
> @@ -36,7 +36,7 @@ import org.junit.jupiter.api.Test;
> /**
> * Unit tests for {@link SmtpManager}.
> */
> -class SmtpManagerTest {
> +public class SmtpManagerTest {
>
> @Test
> void testCreateManagerName() {
> diff --git a/log4j-core/src/test/java/org/apache/logging/log4j/core/net/SocketAppenderReconnectTest.java b/log4j-core/src/test/java/org/apache/logging/log4j/core/net/SocketAppenderReconnectTest.java
> index 5fa603f..87f30dd 100644
> --- a/log4j-core/src/test/java/org/apache/logging/log4j/core/net/SocketAppenderReconnectTest.java
> +++ b/log4j-core/src/test/java/org/apache/logging/log4j/core/net/SocketAppenderReconnectTest.java
> @@ -55,7 +55,7 @@ import static org.junit.jupiter.api.Assertions.*;
> /**
> * Tests reconnection support of {@link org.apache.logging.log4j.core.appender.SocketAppender}.
> */
> -class SocketAppenderReconnectTest {
> +public class SocketAppenderReconnectTest {
>
> private static final Logger LOGGER = StatusLogger.getLogger();
>
Re: [logging-log4j2] 02/04: Our convention is to make test classes public.
Posted by Gary Gregory <ga...@gmail.com>.
Hi Matt,
Porting to Junit 5 would be nice. I'm not sure how to deal with all our
Junit 4 rules though.
Gary
On Wed, Jan 12, 2022, 13:07 Matt Sicker <bo...@gmail.com> wrote:
> I'll note that the convention from JUnit 4 is to make them public;
> JUnit 5 encourages package-private tests instead for some reason, and
> that's the default template for JUnit 5 tests in IntelliJ. I do like
> consistency, though!
>
> On Wed, Jan 12, 2022 at 11:01 AM <gg...@apache.org> wrote:
> >
> > This is an automated email from the ASF dual-hosted git repository.
> >
> > ggregory pushed a commit to branch release-2.x
> > in repository https://gitbox.apache.org/repos/asf/logging-log4j2.git
> >
> > commit b4a892acd1d37f355c27cde10ab1736cf1ebe315
> > Author: Gary Gregory <ga...@gmail.com>
> > AuthorDate: Wed Jan 12 11:58:30 2022 -0500
> >
> > Our convention is to make test classes public.
> > ---
> > .../test/java/org/apache/logging/log4j/core/net/SmtpManagerTest.java
> | 2 +-
> > .../org/apache/logging/log4j/core/net/SocketAppenderReconnectTest.java
> | 2 +-
> > 2 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git
> a/log4j-core/src/test/java/org/apache/logging/log4j/core/net/SmtpManagerTest.java
> b/log4j-core/src/test/java/org/apache/logging/log4j/core/net/SmtpManagerTest.java
> > index df98702..5759cf7 100644
> > ---
> a/log4j-core/src/test/java/org/apache/logging/log4j/core/net/SmtpManagerTest.java
> > +++
> b/log4j-core/src/test/java/org/apache/logging/log4j/core/net/SmtpManagerTest.java
> > @@ -36,7 +36,7 @@ import org.junit.jupiter.api.Test;
> > /**
> > * Unit tests for {@link SmtpManager}.
> > */
> > -class SmtpManagerTest {
> > +public class SmtpManagerTest {
> >
> > @Test
> > void testCreateManagerName() {
> > diff --git
> a/log4j-core/src/test/java/org/apache/logging/log4j/core/net/SocketAppenderReconnectTest.java
> b/log4j-core/src/test/java/org/apache/logging/log4j/core/net/SocketAppenderReconnectTest.java
> > index 5fa603f..87f30dd 100644
> > ---
> a/log4j-core/src/test/java/org/apache/logging/log4j/core/net/SocketAppenderReconnectTest.java
> > +++
> b/log4j-core/src/test/java/org/apache/logging/log4j/core/net/SocketAppenderReconnectTest.java
> > @@ -55,7 +55,7 @@ import static org.junit.jupiter.api.Assertions.*;
> > /**
> > * Tests reconnection support of {@link
> org.apache.logging.log4j.core.appender.SocketAppender}.
> > */
> > -class SocketAppenderReconnectTest {
> > +public class SocketAppenderReconnectTest {
> >
> > private static final Logger LOGGER = StatusLogger.getLogger();
> >
>
Re: [logging-log4j2] 02/04: Our convention is to make test classes public.
Posted by Gary Gregory <ga...@gmail.com>.
Good to hear. I've stubbed my toes going from Junit rules 4 to Junit 5
extensions. I don't want to take the time to do that now though.
Gary
On Wed, Jan 12, 2022, 14:42 Matt Sicker <bo...@gmail.com> wrote:
> Most of the JUnit tests can be mechanically translated via IntelliJ
> (or other tools potentially). The tests that need manual migration are
> ones that use an expected exception in the @Test annotation and tests
> that use rules. I've already ported most of the JUnit 4 rules into
> equivalent JUnit 5 extensions, though they work a little differently
> (e.g., instead of invoking a rule instance to get objects from the
> LoggerContext, you can add a LoggerContext parameter to your test
> method and have it injected).
>
> On Wed, Jan 12, 2022 at 12:46 PM Gary Gregory <ga...@gmail.com>
> wrote:
> >
> > I happy to stick with Junit 5 convertions once we drop Junit 4, which
> feels
> > like a tediuous big job :-(
> >
> > Gafy
> >
> > On Wed, Jan 12, 2022, 13:33 Carter Kozak <ck...@ckozak.net> wrote:
> >
> > > +1
> > >
> > > I prefer minimum visibility by default for the same reason I prefer to
> > > make everything final by default: It gives us more freedom to change
> later
> > > on. This doesn't directly apply to tests, but it's nice when a
> convention
> > > applies globally.
> > >
> > > Most projects don't make junit5 tests public, so there's a question of
> > > whether we want to be consistent with our own usage of junit4, or with
> > > broader usage of junit5. I prefer the latter. We could enforce it with
> > > error-prone for consistency, if desired.
> > >
> > > -ck
> > >
> > > On Wed, Jan 12, 2022, at 13:07, Matt Sicker wrote:
> > > > I'll note that the convention from JUnit 4 is to make them public;
> > > > JUnit 5 encourages package-private tests instead for some reason, and
> > > > that's the default template for JUnit 5 tests in IntelliJ. I do like
> > > > consistency, though!
> > > >
> > > > On Wed, Jan 12, 2022 at 11:01 AM <gg...@apache.org> wrote:
> > > > >
> > > > > This is an automated email from the ASF dual-hosted git repository.
> > > > >
> > > > > ggregory pushed a commit to branch release-2.x
> > > > > in repository
> https://gitbox.apache.org/repos/asf/logging-log4j2.git
> > > > >
> > > > > commit b4a892acd1d37f355c27cde10ab1736cf1ebe315
> > > > > Author: Gary Gregory <ga...@gmail.com>
> > > > > AuthorDate: Wed Jan 12 11:58:30 2022 -0500
> > > > >
> > > > > Our convention is to make test classes public.
> > > > > ---
> > > > >
> .../test/java/org/apache/logging/log4j/core/net/SmtpManagerTest.java
> > > | 2 +-
> > > > >
> > >
> .../org/apache/logging/log4j/core/net/SocketAppenderReconnectTest.java | 2
> > > +-
> > > > > 2 files changed, 2 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git
> > >
> a/log4j-core/src/test/java/org/apache/logging/log4j/core/net/SmtpManagerTest.java
> > >
> b/log4j-core/src/test/java/org/apache/logging/log4j/core/net/SmtpManagerTest.java
> > > > > index df98702..5759cf7 100644
> > > > > ---
> > >
> a/log4j-core/src/test/java/org/apache/logging/log4j/core/net/SmtpManagerTest.java
> > > > > +++
> > >
> b/log4j-core/src/test/java/org/apache/logging/log4j/core/net/SmtpManagerTest.java
> > > > > @@ -36,7 +36,7 @@ import org.junit.jupiter.api.Test;
> > > > > /**
> > > > > * Unit tests for {@link SmtpManager}.
> > > > > */
> > > > > -class SmtpManagerTest {
> > > > > +public class SmtpManagerTest {
> > > > >
> > > > > @Test
> > > > > void testCreateManagerName() {
> > > > > diff --git
> > >
> a/log4j-core/src/test/java/org/apache/logging/log4j/core/net/SocketAppenderReconnectTest.java
> > >
> b/log4j-core/src/test/java/org/apache/logging/log4j/core/net/SocketAppenderReconnectTest.java
> > > > > index 5fa603f..87f30dd 100644
> > > > > ---
> > >
> a/log4j-core/src/test/java/org/apache/logging/log4j/core/net/SocketAppenderReconnectTest.java
> > > > > +++
> > >
> b/log4j-core/src/test/java/org/apache/logging/log4j/core/net/SocketAppenderReconnectTest.java
> > > > > @@ -55,7 +55,7 @@ import static org.junit.jupiter.api.Assertions.*;
> > > > > /**
> > > > > * Tests reconnection support of {@link
> > > org.apache.logging.log4j.core.appender.SocketAppender}.
> > > > > */
> > > > > -class SocketAppenderReconnectTest {
> > > > > +public class SocketAppenderReconnectTest {
> > > > >
> > > > > private static final Logger LOGGER = StatusLogger.getLogger();
> > > > >
> > > >
> > >
>
Re: [logging-log4j2] 02/04: Our convention is to make test classes public.
Posted by Matt Sicker <bo...@gmail.com>.
Most of the JUnit tests can be mechanically translated via IntelliJ
(or other tools potentially). The tests that need manual migration are
ones that use an expected exception in the @Test annotation and tests
that use rules. I've already ported most of the JUnit 4 rules into
equivalent JUnit 5 extensions, though they work a little differently
(e.g., instead of invoking a rule instance to get objects from the
LoggerContext, you can add a LoggerContext parameter to your test
method and have it injected).
On Wed, Jan 12, 2022 at 12:46 PM Gary Gregory <ga...@gmail.com> wrote:
>
> I happy to stick with Junit 5 convertions once we drop Junit 4, which feels
> like a tediuous big job :-(
>
> Gafy
>
> On Wed, Jan 12, 2022, 13:33 Carter Kozak <ck...@ckozak.net> wrote:
>
> > +1
> >
> > I prefer minimum visibility by default for the same reason I prefer to
> > make everything final by default: It gives us more freedom to change later
> > on. This doesn't directly apply to tests, but it's nice when a convention
> > applies globally.
> >
> > Most projects don't make junit5 tests public, so there's a question of
> > whether we want to be consistent with our own usage of junit4, or with
> > broader usage of junit5. I prefer the latter. We could enforce it with
> > error-prone for consistency, if desired.
> >
> > -ck
> >
> > On Wed, Jan 12, 2022, at 13:07, Matt Sicker wrote:
> > > I'll note that the convention from JUnit 4 is to make them public;
> > > JUnit 5 encourages package-private tests instead for some reason, and
> > > that's the default template for JUnit 5 tests in IntelliJ. I do like
> > > consistency, though!
> > >
> > > On Wed, Jan 12, 2022 at 11:01 AM <gg...@apache.org> wrote:
> > > >
> > > > This is an automated email from the ASF dual-hosted git repository.
> > > >
> > > > ggregory pushed a commit to branch release-2.x
> > > > in repository https://gitbox.apache.org/repos/asf/logging-log4j2.git
> > > >
> > > > commit b4a892acd1d37f355c27cde10ab1736cf1ebe315
> > > > Author: Gary Gregory <ga...@gmail.com>
> > > > AuthorDate: Wed Jan 12 11:58:30 2022 -0500
> > > >
> > > > Our convention is to make test classes public.
> > > > ---
> > > > .../test/java/org/apache/logging/log4j/core/net/SmtpManagerTest.java
> > | 2 +-
> > > >
> > .../org/apache/logging/log4j/core/net/SocketAppenderReconnectTest.java | 2
> > +-
> > > > 2 files changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git
> > a/log4j-core/src/test/java/org/apache/logging/log4j/core/net/SmtpManagerTest.java
> > b/log4j-core/src/test/java/org/apache/logging/log4j/core/net/SmtpManagerTest.java
> > > > index df98702..5759cf7 100644
> > > > ---
> > a/log4j-core/src/test/java/org/apache/logging/log4j/core/net/SmtpManagerTest.java
> > > > +++
> > b/log4j-core/src/test/java/org/apache/logging/log4j/core/net/SmtpManagerTest.java
> > > > @@ -36,7 +36,7 @@ import org.junit.jupiter.api.Test;
> > > > /**
> > > > * Unit tests for {@link SmtpManager}.
> > > > */
> > > > -class SmtpManagerTest {
> > > > +public class SmtpManagerTest {
> > > >
> > > > @Test
> > > > void testCreateManagerName() {
> > > > diff --git
> > a/log4j-core/src/test/java/org/apache/logging/log4j/core/net/SocketAppenderReconnectTest.java
> > b/log4j-core/src/test/java/org/apache/logging/log4j/core/net/SocketAppenderReconnectTest.java
> > > > index 5fa603f..87f30dd 100644
> > > > ---
> > a/log4j-core/src/test/java/org/apache/logging/log4j/core/net/SocketAppenderReconnectTest.java
> > > > +++
> > b/log4j-core/src/test/java/org/apache/logging/log4j/core/net/SocketAppenderReconnectTest.java
> > > > @@ -55,7 +55,7 @@ import static org.junit.jupiter.api.Assertions.*;
> > > > /**
> > > > * Tests reconnection support of {@link
> > org.apache.logging.log4j.core.appender.SocketAppender}.
> > > > */
> > > > -class SocketAppenderReconnectTest {
> > > > +public class SocketAppenderReconnectTest {
> > > >
> > > > private static final Logger LOGGER = StatusLogger.getLogger();
> > > >
> > >
> >
Re: [logging-log4j2] 02/04: Our convention is to make test classes public.
Posted by Gary Gregory <ga...@gmail.com>.
I happy to stick with Junit 5 convertions once we drop Junit 4, which feels
like a tediuous big job :-(
Gafy
On Wed, Jan 12, 2022, 13:33 Carter Kozak <ck...@ckozak.net> wrote:
> +1
>
> I prefer minimum visibility by default for the same reason I prefer to
> make everything final by default: It gives us more freedom to change later
> on. This doesn't directly apply to tests, but it's nice when a convention
> applies globally.
>
> Most projects don't make junit5 tests public, so there's a question of
> whether we want to be consistent with our own usage of junit4, or with
> broader usage of junit5. I prefer the latter. We could enforce it with
> error-prone for consistency, if desired.
>
> -ck
>
> On Wed, Jan 12, 2022, at 13:07, Matt Sicker wrote:
> > I'll note that the convention from JUnit 4 is to make them public;
> > JUnit 5 encourages package-private tests instead for some reason, and
> > that's the default template for JUnit 5 tests in IntelliJ. I do like
> > consistency, though!
> >
> > On Wed, Jan 12, 2022 at 11:01 AM <gg...@apache.org> wrote:
> > >
> > > This is an automated email from the ASF dual-hosted git repository.
> > >
> > > ggregory pushed a commit to branch release-2.x
> > > in repository https://gitbox.apache.org/repos/asf/logging-log4j2.git
> > >
> > > commit b4a892acd1d37f355c27cde10ab1736cf1ebe315
> > > Author: Gary Gregory <ga...@gmail.com>
> > > AuthorDate: Wed Jan 12 11:58:30 2022 -0500
> > >
> > > Our convention is to make test classes public.
> > > ---
> > > .../test/java/org/apache/logging/log4j/core/net/SmtpManagerTest.java
> | 2 +-
> > >
> .../org/apache/logging/log4j/core/net/SocketAppenderReconnectTest.java | 2
> +-
> > > 2 files changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git
> a/log4j-core/src/test/java/org/apache/logging/log4j/core/net/SmtpManagerTest.java
> b/log4j-core/src/test/java/org/apache/logging/log4j/core/net/SmtpManagerTest.java
> > > index df98702..5759cf7 100644
> > > ---
> a/log4j-core/src/test/java/org/apache/logging/log4j/core/net/SmtpManagerTest.java
> > > +++
> b/log4j-core/src/test/java/org/apache/logging/log4j/core/net/SmtpManagerTest.java
> > > @@ -36,7 +36,7 @@ import org.junit.jupiter.api.Test;
> > > /**
> > > * Unit tests for {@link SmtpManager}.
> > > */
> > > -class SmtpManagerTest {
> > > +public class SmtpManagerTest {
> > >
> > > @Test
> > > void testCreateManagerName() {
> > > diff --git
> a/log4j-core/src/test/java/org/apache/logging/log4j/core/net/SocketAppenderReconnectTest.java
> b/log4j-core/src/test/java/org/apache/logging/log4j/core/net/SocketAppenderReconnectTest.java
> > > index 5fa603f..87f30dd 100644
> > > ---
> a/log4j-core/src/test/java/org/apache/logging/log4j/core/net/SocketAppenderReconnectTest.java
> > > +++
> b/log4j-core/src/test/java/org/apache/logging/log4j/core/net/SocketAppenderReconnectTest.java
> > > @@ -55,7 +55,7 @@ import static org.junit.jupiter.api.Assertions.*;
> > > /**
> > > * Tests reconnection support of {@link
> org.apache.logging.log4j.core.appender.SocketAppender}.
> > > */
> > > -class SocketAppenderReconnectTest {
> > > +public class SocketAppenderReconnectTest {
> > >
> > > private static final Logger LOGGER = StatusLogger.getLogger();
> > >
> >
>
Re: [logging-log4j2] 02/04: Our convention is to make test classes public.
Posted by Carter Kozak <ck...@ckozak.net>.
+1
I prefer minimum visibility by default for the same reason I prefer to make everything final by default: It gives us more freedom to change later on. This doesn't directly apply to tests, but it's nice when a convention applies globally.
Most projects don't make junit5 tests public, so there's a question of whether we want to be consistent with our own usage of junit4, or with broader usage of junit5. I prefer the latter. We could enforce it with error-prone for consistency, if desired.
-ck
On Wed, Jan 12, 2022, at 13:07, Matt Sicker wrote:
> I'll note that the convention from JUnit 4 is to make them public;
> JUnit 5 encourages package-private tests instead for some reason, and
> that's the default template for JUnit 5 tests in IntelliJ. I do like
> consistency, though!
>
> On Wed, Jan 12, 2022 at 11:01 AM <gg...@apache.org> wrote:
> >
> > This is an automated email from the ASF dual-hosted git repository.
> >
> > ggregory pushed a commit to branch release-2.x
> > in repository https://gitbox.apache.org/repos/asf/logging-log4j2.git
> >
> > commit b4a892acd1d37f355c27cde10ab1736cf1ebe315
> > Author: Gary Gregory <ga...@gmail.com>
> > AuthorDate: Wed Jan 12 11:58:30 2022 -0500
> >
> > Our convention is to make test classes public.
> > ---
> > .../test/java/org/apache/logging/log4j/core/net/SmtpManagerTest.java | 2 +-
> > .../org/apache/logging/log4j/core/net/SocketAppenderReconnectTest.java | 2 +-
> > 2 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/log4j-core/src/test/java/org/apache/logging/log4j/core/net/SmtpManagerTest.java b/log4j-core/src/test/java/org/apache/logging/log4j/core/net/SmtpManagerTest.java
> > index df98702..5759cf7 100644
> > --- a/log4j-core/src/test/java/org/apache/logging/log4j/core/net/SmtpManagerTest.java
> > +++ b/log4j-core/src/test/java/org/apache/logging/log4j/core/net/SmtpManagerTest.java
> > @@ -36,7 +36,7 @@ import org.junit.jupiter.api.Test;
> > /**
> > * Unit tests for {@link SmtpManager}.
> > */
> > -class SmtpManagerTest {
> > +public class SmtpManagerTest {
> >
> > @Test
> > void testCreateManagerName() {
> > diff --git a/log4j-core/src/test/java/org/apache/logging/log4j/core/net/SocketAppenderReconnectTest.java b/log4j-core/src/test/java/org/apache/logging/log4j/core/net/SocketAppenderReconnectTest.java
> > index 5fa603f..87f30dd 100644
> > --- a/log4j-core/src/test/java/org/apache/logging/log4j/core/net/SocketAppenderReconnectTest.java
> > +++ b/log4j-core/src/test/java/org/apache/logging/log4j/core/net/SocketAppenderReconnectTest.java
> > @@ -55,7 +55,7 @@ import static org.junit.jupiter.api.Assertions.*;
> > /**
> > * Tests reconnection support of {@link org.apache.logging.log4j.core.appender.SocketAppender}.
> > */
> > -class SocketAppenderReconnectTest {
> > +public class SocketAppenderReconnectTest {
> >
> > private static final Logger LOGGER = StatusLogger.getLogger();
> >
>