Use Gradle's Worker API #32
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@larsgrefer very cool :-) Can you agree to the terms of the Gradle CPD plugin Contributor License Agreement, also for this PR? I will review it asap. |
|
@aaschmid Some of the Tests in |
|
@larsgrefer hm ... yes I am aware of that change - seems that Gradle does not like unit tests ... However, another way is to get the actions of the task and execute these actions. Do you want to try? |
|
@larsgrefer Thanks for the work. It looks very well, already. Please don't be angry / afraid about all the review comments, I am happy to discuss them or even implement them myself if you prefer :-) |
Some of the tests (mostly the ones testing successful cases) could be refactored to directly use the new |
aaschmid
requested changes
Apr 6, 2019
| languageProperties, | ||
| isSkipLexicalErrors(), | ||
| isSkipDuplicateFiles(), | ||
| new HashSet<>(getSource().getFiles()), |
There was a problem hiding this comment.
Would it be possible to hand over an Collections.unmodifiableSet?
There was a problem hiding this comment.
It would be possible, but what would be the benefit?
IMHO an unmodifiable set is only interesting for sets which are used for a long time and which are accessed/shared by multiple components.
There was a problem hiding this comment.
Hm ... I thought more about the performance as copying the set is more expensive, even more on hugh projects ...
| isSkipLexicalErrors(), | ||
| isSkipDuplicateFiles(), | ||
| new HashSet<>(getSource().getFiles()), | ||
| new ArrayList(enabledReports), |
| reporter.generate(matches) | ||
| logResult(matches) | ||
| } | ||
| if (isIgnoreLiterals()) { |
There was a problem hiding this comment.
Can these be always set using Boolean.toString as for Tokenizer.OPTION_SKIP_BLOCKS?
There was a problem hiding this comment.
I'm not sure, I've just took the logic from here:
| } else { | ||
| throw new GradleException(message); | ||
| } | ||
| if (enabledReports.isEmpty()) { |
There was a problem hiding this comment.
hm ... we assume that it is no use case to only have a console output without details in a report, don't we?
There was a problem hiding this comment.
This test "wants" the task to throw an exception in this case, thats why I built it that way.
| } | ||
| else { | ||
| String message = "CPD found duplicate code."; | ||
| SingleFileReport report = reports.get(0); |
There was a problem hiding this comment.
Should always return a report with the check and throw in Cpd.
| String message = "CPD found duplicate code."; | ||
| SingleFileReport report = reports.get(0); | ||
| if (report != null) { | ||
| String reportUrl = report.getDestination().toString(); |
There was a problem hiding this comment.
What about the clickabeFileUrl?
| @@ -130,28 +130,6 @@ class CpdAcceptanceTest extends BaseSpec { | |||
| e.cause.message == '''Task 'cpdCheck' requires exactly one report to be enabled but was: [].''' | |||
| } | |||
|
|
|||
| def "executing 'Cpd' task throws wrapped 'InvalidUserDataException' if more than one report is enabled"() { | |||
There was a problem hiding this comment.
Would it make sense to replace with an (integration-)test which enables two outputs and verify this?
aaschmid
requested changes
Apr 6, 2019
There was a problem hiding this comment.
Thanks for the PR @larsgrefer. Would you mind to discuss my comments or change the code?
|
After merging the code, tests fail with an exception like the following. I will investigate further ... |
|
Problem is that properties of |
Closed
larsgrefer
added a commit
to larsgrefer/gradle-cpd-plugin
that referenced
this pull request
Apr 7, 2019
|
Problem is that properties of Now a |
aaschmid
pushed a commit
that referenced
this pull request
Apr 22, 2019
aaschmid
added a commit
that referenced
this pull request
Apr 22, 2019
aaschmid
added a commit
that referenced
this pull request
Apr 22, 2019
aaschmid
added a commit
that referenced
this pull request
May 10, 2019
* use-worker-api: add JDKs 10 and 11 for travis-ci test minimal possible toolVersion and fallback language remove deprecated API adjust minimal required PMD version create executor configuration and move to dedicated package (#38) add check methods for state of task before actually execute anything use serializable report configuration for CpdAction (#38) remove no longer required excludes remove unnecessary loggers for report implementations Use Gradle's Worker API (#32) use 'getByName' consistent to other test cases remove @optional for non-optional properties add integration test which really executes CPD add both test sourceSets to gradlePlugin
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This removes the need for the classloading hack mentioned here:
gradle-cpd-plugin/src/main/groovy/de/aaschmid/gradle/plugins/cpd/Cpd.groovy
Line 183 in ed44ed7
As addition, the task can now run parallel to other tasks, and multiple reports can be generated at once.
See also: https://guides.gradle.org/using-the-worker-api/