Junit5: Consider static factory methods instead of constructors

[jbduncan]

Overview

Currently, there aren't many classes (if any) in JUnit 5 that are instantiated via _static factory methods_, as opposed to constructors. _Effective Java 2nd Edition_, Item 1 gives a number of compelling reasons for preferring static factory methods. Therefore we should consider adopting them more, skipping over those classes where there is a more compelling reason for them to have public constructors instead.

Deliverables

• [x] Evaluate which classes should have public _static factory methods_.
• [x] For the following classes, make their constructors private or protected and introduce static factory methods (SFMs) in their place.
  
  
  
  • [x] ClasspathResourceSource
  
  
  • [x] ClassSource
  
  
  • [x] CompositeTestSource
  
  
  • [x] DirectorySource
  
  
  • [x] FilePosition
  
  
  • [x] FileSource
  
  
  • [x] MethodSource
  
  
  • [x] PackageSource
  
  
  • [x] LoggingListener: make the existing constructor private and introduce a forBiConsumer(...) SFM.
  
  
• ❌ Introduce a new TestSources utility class (analogous to DiscoverySelectors) that will house the SFMs for TestSource implementations.
  
  
  
  • The team decided to introduce from(...) SFMs in each TestSource implementation instead.
  
  

[sbrannen]

slated for RC1 for team discussion

[jbduncan]

A possible regex to use to search for candidate classes (at least within IntelliJ IDEA):

public\s*(?!.*final|.*abstract)\s*class

[sbrannen]

That RegEx also works in Eclipse.

[sbrannen]

@jbduncan, do you have any particular classes in mind?

[sbrannen]

Currently, there aren't many classes (if any) in JUnit 5 that are instantiated via static factory methods, as opposed to constructors.

That's actually not true.

We have several classes with static factory methods, and in other situations the constructor is package private so that no third party could invoke them anyway.

[jbduncan]

@sbrannen

That's actually not true.

We have several classes with static factory methods, and in other situations the constructor is package private so that no third party could invoke them anyway.

Oh yes, you're right! I admit I didn't search thoroughly for offending classes when I opened the issue, so apologies.

@jbduncan, do you have any particular classes in mind?

I've identified the following list of classes which I think should be considered for the static factory method treatment.

Classes labelled with (?) are classes that I'm unsure about due to my knowledge on how JUnit 5 instantiates them being incomplete.

Although I've listed LoggingListener, which does already has some static factory methods, I notice it still has a single public constructor new LoggingListener(BiConsumer) which I think could be renamed to LoggingListener.forCustomLogger(BiConsumer) or something similar.

Furthermore, although I've also listed ToStringBuilder, I do realise that it's an internal class, so I wouldn't be too upset if it weren't given static factory method treatment for that reason. :wink:

ClassSource
CompositeTestSource
DirectorySource
DynamicContainer
ExpectedExceptionSupport (?)
ExternalResourceSupport (?)
FilePosition
FileSource
JUnitPlatform
LoggingListener
MethodSource
PackageSource
SingleTestExecutor
SummaryGeneratingListener
ToStringBuilder
VerifierSupport (?)

[marcphilipp]

I also went through the (non-internal) API. Here are my lists:

Static Factory Methods

• DynamicContainer
• DynamicTest
• ConditionEvaluationResult
• FilterResult
• TestExecutionResult
• UniqueId
• ClasspathResourceSelector
• ClasspathRootSelector
• ClassSelector
• DirectorySelector
• FileSelector
• MethodSelector
• PackageSelector
• UniqueIdSelector
• UriSelector
• EngineFilter
• TagFilter
• ClassNameFilter
• PackageNameFilter
• ConsoleLauncherExecutionResult

Constructors

• ExtensionConfigurationException
• ExtensionContextException
• JUnitException
• PreconditionViolationException
• ArgumentConversionException
• ToStringBuilder
• ExecutionRequest (internal constructor)
• ClasspathResourceSource
• ClassSource
• CompositeTestSource
• DirectorySource
• FileSource
• FilePosition
• MethodSource
• PackageSource
• EngineDescriptor
• LoggingListener (both constructor and static factory method)
• SummaryGeneratingListener

[jbduncan]

Sorry @marcphilipp, I'm not entirely clear on the purpose of the lists above.

Are they identifying _existing_ classes which use either static factory methods (SFMs) or constructors, or are they identifying which classes you _think_ should have SFMs and which should have constructors?

[marcphilipp]

ClassSource
CompositeTestSource
DirectorySource
FilePosition
FileSource
MethodSource
PackageSource

For these I think it might make sense to use static factory methods. @junit-team/junit-lambda Thoughts?

Already has static factory methods and a private constructor, doesn't it?

ExpectedExceptionSupport (?)
ExternalResourceSupport (?)
VerifierSupport (?)

These are instantiated by JUnit 4 and thus need a public constructor.

SummaryGeneratingListener

These do not declare a constructor and I don't foresee us adding one so they should be fine.

Has a single constructor that looks like in other libraries with similar helper classes. Thus, I'd rather keep it that way. It's already a builder, so if we ever needed to add an additional parameter we could add a builder method.

I think this one should definitely be changed because it uses both static factory methods and has a public constructor.

[marcphilipp]

Are they identifying _existing_ classes which use either static factory methods (SFMs) or constructors, or are they identifying which classes you _think_ should have SFMs and which should have constructors?

Point taken. They identify _existing_ classes which use either static factory methods (SFMs) or constructors.

[jbduncan]

DynamicContainer
Already has static factory methods and a private constructor, doesn't it?

@marcphilipp Oh yep, you're correct. Not sure why I added it to the list. :stuck_out_tongue:

[sbrannen]

ClassSource
CompositeTestSource
DirectorySource
FilePosition
FileSource
MethodSource
PackageSource

For these I think it might make sense to use static factory methods. @junit-team/junit-lambda Thoughts?

I'm OK with making that change.

[sbrannen]

... by which I mean: introducing from(...) SFMs in concrete TestSource implementations.

[sbrannen]

ExpectedExceptionSupport (?)
ExternalResourceSupport (?)
VerifierSupport (?)

These are instantiated by JUnit 4 and thus need a public constructor.

Well, _technically_ only the JUnitPlatform is instantiated by JUnit 4. The others are instantiated by JUnit Jupiter and therefore only require a no-args constructor (which need not be public).

But I nonetheless agree that we cannot introduce SFMs for those. 😉

[sbrannen]

SummaryGeneratingListener

These do not declare a constructor and I don't foresee us adding one so they should be fine.

Agreed.

[sbrannen]

Has a single constructor that looks like in other libraries with similar helper classes. Thus, I'd rather keep it that way. It's already a builder, so if we ever needed to add an additional parameter we could add a builder method.

Agreed.

[sbrannen]

I think this one should definitely be changed because it uses both static factory methods and has a public constructor.

Agreed.

We should make the existing constructor private and introduce a new forBiConsumer(...) SFM.

[sbrannen]

_in progress_

[sbrannen]

Updated _Deliverables_.

[sbrannen]

FYI: current work can be viewed in the following branch.

https://github.com/junit-team/junit5/tree/issues/939-static-factory-methods

[sbrannen]

Update:

All changes are complete and have been pushed to the aforementioned branch for review.

[sbrannen]

Update:

Discussion should take place in PR #1046 instead of on the branch.

[sbrannen]

Closing this issue since #1046 has been merged into master.

[jbduncan]

@sbrannen @marcphilipp The results of #1046 LGTM now. Many thanks for taking the time and effort to address this! 😀