Skip to content

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

Merged
Tibor17 merged 1 commit intoapache:masterfrom
wilkinsona:SUREFIRE-1679
Aug 14, 2019
Merged

[SUREFIRE-1679] Prevent classpath caching from causing pollution#243
Tibor17 merged 1 commit intoapache:masterfrom
wilkinsona:SUREFIRE-1679

Conversation

@wilkinsona
Copy link
Copy Markdown
Contributor

@wilkinsona wilkinsona commented Aug 14, 2019

This PR should fix SUREFIRE-1679. Once the changes have been reviewed and approved, I'll open another PR to backport them to 2.x.

@Tibor17
Copy link
Copy Markdown
Contributor

Tibor17 commented Aug 14, 2019

@wilkinsona
Thx, LGTM
I will make ASF branch and trigger Jenkins.

@Tibor17 Tibor17 self-requested a review August 14, 2019 08:49
@Tibor17 Tibor17 self-assigned this Aug 14, 2019
@Tibor17 Tibor17 removed their request for review August 14, 2019 08:49
@Tibor17
Copy link
Copy Markdown
Contributor

Tibor17 commented Aug 14, 2019

The build is currently running
https://builds.apache.org/job/maven-box/job/maven-surefire/job/SUREFIRE-1679/

@Tibor17
Copy link
Copy Markdown
Contributor

Tibor17 commented Aug 14, 2019 •
edited
Loading

@wilkinsona
Can you please add unit tests and fully cover your cache?
I want to keep the code coverage on current level at least or higher.

@Tibor17
Copy link
Copy Markdown
Contributor

Tibor17 commented Aug 14, 2019

@wilkinsona
There is checkstyle errors. You can see this if you build the project locally mvn install.

11:05:39  [INFO] --- maven-checkstyle-plugin:3.0.0:check (checkstyle-check) @ maven-surefire-common ---
11:05:41  [INFO] There are 5 errors reported by Checkstyle 6.18 with config/maven_checks.xml ruleset.
11:05:41  [ERROR] src\main\java\org\apache\maven\plugin\surefire\AbstractSurefireMojo.java:[3856,9] (whitespace) FileTabCharacter: Line contains a tab character.
11:05:41  [INFO] Ignored 4 errors, 1 violation remaining.

@wilkinsona
Copy link
Copy Markdown
Contributor Author

wilkinsona commented Aug 14, 2019

Oops. Sorry. I've force-pushed a fix for that. I'll take a look at unit tests shortly. Given that the old ClasspathCache is now unused (as far as I can tell), I'm not sure that it'll be possible to maintain the same level of coverage. However, I can at least try to add a test that verifies the pollution has been fixed.

@olamy
Copy link
Copy Markdown
Member

olamy commented Aug 14, 2019

@wilkinsona read this one https://stackoverflow.blog/2017/06/15/developers-use-spaces-make-money-use-tabs/ :)

@Tibor17
Copy link
Copy Markdown
Contributor

Tibor17 commented Aug 14, 2019

@wilkinsona
The cache has ConcurrentHashMap.
If you agree, this Map can be turned to the pure HashMap.
At least I do not see any multithreading in this MOJO class.

@wilkinsona
[SUREFIRE-1679] Prevent classpath caching from causing pollution
0cf43c9 
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.
@wilkinsona
Copy link
Copy Markdown
Contributor Author

wilkinsona commented Aug 14, 2019 •
edited
Loading

I've force-pushed the switch to a HashMap and the addition of a unit test.

@Tibor17
Copy link
Copy Markdown
Contributor

Tibor17 commented Aug 14, 2019

@wilkinsona
thx, the build is in progress.

@Tibor17
Copy link
Copy Markdown
Contributor

Tibor17 commented Aug 14, 2019

@wilkinsona Thx for contributing!

@Tibor17 Tibor17 merged commit f7d4310 into apache:master Aug 14, 2019
@wilkinsona wilkinsona deleted the SUREFIRE-1679 branch August 14, 2019 19:08
@wilkinsona
Copy link
Copy Markdown
Contributor Author

wilkinsona commented Aug 15, 2019

My pleasure. Thanks for the review and merge. I'd like to work on an equivalent 2.22.x fix now. What branch should I use as the base for that?

@sormuras
Copy link
Copy Markdown
Contributor

sormuras commented Aug 15, 2019

iirc, it's this branch https://github.com/apache/maven-surefire/tree/release/2.22.2

@wilkinsona wilkinsona mentioned this pull request Aug 15, 2019
Merged
@wilkinsona
Copy link
Copy Markdown
Contributor Author

wilkinsona commented Aug 15, 2019

Thanks, @sormuras. I've opened a PR using that branch as its base: #244.

@jira-importer
Copy link
Copy Markdown

jira-importer commented Jul 4, 2025

Resolve #2092

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

Reviewers

No reviews

Assignees

@Tibor17 Tibor17

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@wilkinsona @Tibor17 @olamy @sormuras @jira-importer