Skip to content

[SUREFIRE-1679] Prevent classpath caching from causing pollution#244

Merged
Tibor17 merged 1 commit intoapache:release/2.22.2from
wilkinsona:SUREFIRE-1679
Oct 17, 2019
Merged

[SUREFIRE-1679] Prevent classpath caching from causing pollution#244
Tibor17 merged 1 commit intoapache:release/2.22.2from
wilkinsona:SUREFIRE-1679

Conversation

@wilkinsona
Copy link
Copy Markdown
Contributor

This pull request backports the changes made in #243 to 2.22.x. The main source changes are the same. The test has be modified to align with the small differences in the implementation. Most notably, I have introduced the use of TemporaryFolder as the use of a mocked File resulted in an NPE as the path field of the mocked File was null. I hope that's OK.

Previously, classpath caching was performed statically. This resulted
in the classpath cached by one project for a particular provider
being used by a subsequent project. As a result any customizations to
the classpath, such as removing duplicate artifacts, would leak out
and pollute the classpath used by subsequent projects.

This commit prevents the pollution by making the classpath cache
instance-scoped so that the cache is only used by a single mojo and,
therefore, a single project.

Backports f7d4310 to 2.22.x
Copy link
Copy Markdown
Contributor

@sormuras sormuras left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Changes made to the test case are minimal - less mocking is a good thing.

@Tibor17
Copy link
Copy Markdown
Contributor

Tibor17 commented Aug 15, 2019

it's okay in version 2.x since these tests must be older and different from 3.x. The reason why there is File mock is to simplify the Captures of debug logger because there is no physicla path and creating real Jar files, no troubles with platform and long lines in code. Also it is a signal for us to fail the test if the logic in AbstractSurefireMojo would change in the future when the logic would need to have a real paths and real reads/writes. Till that it is just in memory.

These tests in AbstractSurefireMojoTest.java do not have too much assertion statements. They have more Captures and verifications. And the reason is that the class AbstractSurefireMojo was not initially developed with tests. Basically there was no TDD in this class development. If there was, I am sure there could be much more assertions on returned values and less mocking of internal state and captures. This happens when TDD is not applied in the begining. Positive remark is that these tests improve the coverage and give us some feedback when the code is changed together with the experiences of the AbstractSurefireMojo.

@Tibor17 Tibor17 self-requested a review August 15, 2019 18:25
@Tibor17
Copy link
Copy Markdown
Contributor

Tibor17 commented Aug 15, 2019

LGTM

@Tibor17 Tibor17 merged commit 7d51f96 into apache:release/2.22.2 Oct 17, 2019
@jira-importer
Copy link
Copy Markdown

Resolve #2092

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants