Reduce duplication

[beatngu13]

This is an early draft to fix #180.

In order to reduce the code duplication among SystemPropertyExtension and EnvironmentVariableExtension, I thought about an abstract base class for entry-based extensions, where entries (i.e. key-value pairs) shall be cleared or set.

As outlined above, this is a draft. There may be still some design flaws, the Javadoc is incomplete, no type wildcards … I just wanted to get some feedback if you think it is worth to further work on this, use a different approach or leave the extensions as they are. ✌️

Personally, I quite like it because when you look at SystemPropertyExtension and EnvironmentVariableExtension now, they are dead-simple.

---

I hereby agree to the terms of the JUnit Pioneer Contributor License Agreement.

[Bukama]

Wow - nice duplicate code reduction so far! Like the approach and interested in how you go further

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
5 Code Smells

96.2% Coverage
0.0% Duplication

[beatngu13]

I think I'm done for the most part. I would keep AbstractEntryBasedExtension internal (package private), but in the future this may become public if users want to extend it for similar purposes.

Note that subclasses of AbstractEntryBasedExtension are currently responsible for discovering their own annotations. In a second step, we might move additional code into AbstractEntryBasedExtension. However, for now I think the way it is implemented is already a good improvement.

Proposed commit message:

Reduce duplication in envar and sysprop extensions

This PR reduces the duplication among `EnvironmentVariableExtension` and `SystemPropertyExtension` by introducing an abstract base class (`AbstractEntryBasedExtension`) for entry-based extensions, where entries (key-value pairs) can be cleared or set.

Closes: #180
PR: #248

[Bukama]

Will have a look, hope tomorrow

[Bukama]

As before - like it very much :)

[beatngu13]

Open question: As with most traditional refactorings, I didn't touch existing tests. But do we want to have some additional tests for the new base class?

[Bukama]

Open question: As with most traditional refactorings, I didn't touch existing tests. But do we want to have some additional tests for the new base class?

I vote for yes. And if there are existing tests of the old extension which, in core, test the same, because the functionality moved to the base class, than we should merge them into tests for the base class.

[aepfli]

lgtm :) thank you

[Bukama]

Open question: As with most traditional refactorings, I didn't touch existing tests. But do we want to have some additional tests for the new base class?

I asked @beatngu13 if he wants to add tests for the base class and we wants so. So we wait for an update :)

[nipafx]

When I saw that we need six abstract methods, I was worried that the abstraction would the very fragile, bit it looks pretty good. Thank you very much @beatngu13! 👍 I was a bit surprised by the methods returning functional interfaces and turned that into "normal" methods (0c72122) - hope that's ok with you.

From my perspective, this can be merged.

[Bukama]

From my perspective, this can be merged.

@beatngu13 wanted to add some additional tests. After that is done we can merge.

[nipafx]

I looked at this for a few minutes, wondering what tests to add.

With abstract base classes, I usually try to come up with an abstract base class for the tests as well. The concrete classes can then be tested by concrete test classes that extend the base class. In that scenario, actual tests are usually only in the abstract test base class and concrete test classes only implement some factory methods. I don't think this will work here because I know of no way to handle the annotations (e.g. @SetEnvironmentVariable) - whether you try to put them into the abstract base class or the implementation, there are weird consequences.

I'm ok with just testing the implementation, i.e. just EnvironmentVariableExtension and SystemPropertyExtension without any tests for the abstract base class.

If you still want to add tests, keep in mind that we don't want to "overtest", i.e. if there's a bug in the abstract base class, we don't need its tests and the implementations to fail if that just adds more failing tests without providing more insight.

[beatngu13]

@nicolaiparlog makes sense, I'm fine with that. 👍

Just did a rebase to current master, so feel free to merge.

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 1 Security Hotspot to review)
0 Code Smells

96.2% Coverage
0.0% Duplication